From 6925503a10af1c95d1e2a37eff43e8241382cf58 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman <rjeffman@redhat.com> Date: Wed, 29 Jan 2025 00:32:16 -0300 Subject: [PATCH] ipagroup: Fix management of AD objects When using AD objects, a user expects to use the more human readable form, like "user@ad.domain", but this impose some dificulties on evaluating which object is being referenced as AD has several forms to refer to the same object. Each object is AD is identified uniquely by its SID, and this is the identifier that IPA stores in its database. When managing AD objects, IPA finds its SID and works with that value. ansible-freeipa tried to process these objects using the human readable values, and it cause idempontence error when ensuring the values were present or modified, and, at least in some cases, prevented the objects to be made absent, as the object list created didn't match the SID to the value used as module parameter. By using SID to process the AD objects in ipagroup, the addition or removal of members works and idempotence of these members is ensured. The only issue with thils approach is that it only works no server nodes. In client nodes, the conversion to SID is not available and the same issues that existed before will still be present. Tests were updated to reflect these changes, a new test, specific to idempotence issues of AD objects was added: tests/group/test_group_ad_users.yml Resolves: https://issues.redhat.com/browse/RHEL-70023 --- README-group.md | 15 ++- plugins/modules/ipagroup.py | 76 +++++++++++---- tests/group/test_group.yml | 30 +++++- tests/group/test_group_ad_users.yml | 73 ++++++++++++++ tests/group/test_group_client_context.yml | 13 +++ tests/group/test_group_external_members.yml | 100 +++++++++++++++----- tests/group/test_group_idoverrideuser.yml | 73 +++++++------- 7 files changed, 295 insertions(+), 85 deletions(-) create mode 100644 tests/group/test_group_ad_users.yml diff --git a/README-group.md b/README-group.md index 75b8bb54..05fd6054 100644 --- a/README-group.md +++ b/README-group.md @@ -8,8 +8,12 @@ The group module allows to ensure presence and absence of groups and members of The group module is as compatible as possible to the Ansible upstream `ipa_group` module, but additionally offers to add users to a group and also to remove users from a group. -## Note -Ensuring presence (adding) of several groups with mixed types (`external`, `nonposix` and `posix`) requires a fix in FreeIPA. The module implements a workaround to automatically use `client` context if the fix is not present in the target node FreeIPA and if more than one group is provided to the task using the `groups` parameter. If `ipaapi_context` is forced to be `server`, the module will fail in this case. + +Notes +----- + +* Ensuring presence (adding) of several groups with mixed types (`external`, `nonposix` and `posix`) requires a fix in FreeIPA. The module implements a workaround to automatically use `client` context if the fix is not present in the target node FreeIPA and if more than one group is provided to the task using the `groups` parameter. If `ipaapi_context` is forced to be `server`, the module will fail in this case. +* Using `externalmember` or `idoverrideuser` is only supported with `ipaapi_context: server`. With 'client' context, module execution will fail. Features @@ -213,7 +217,7 @@ Example playbook to add members from a trusted realm to an external group: --- - name: Playbook to handle groups. hosts: ipaserver - + tasks: - name: Create an external group and add members from a trust to it. ipagroup: @@ -276,6 +280,7 @@ Example playbook to ensure groups are absent: state: absent ``` + Variables ========= @@ -299,8 +304,8 @@ Variable | Description | Required `service` | List of service name strings assigned to this group. Only usable with IPA versions 4.7 and up. | no `membermanager_user` | List of member manager users assigned to this group. Only usable with IPA versions 4.8.4 and up. | no `membermanager_group` | List of member manager groups assigned to this group. Only usable with IPA versions 4.8.4 and up. | no -`externalmember` \| `ipaexternalmember` \| `external_member`| List of members of a trusted domain in DOM\\name or name@domain form. | no -`idoverrideuser` | List of user ID overrides to manage. Only usable with IPA versions 4.8.7 and up.| no +`externalmember` \| `ipaexternalmember` \| `external_member`| List of members of a trusted domain in DOM\\name or name@domain form. Requires "server" context. | no +`idoverrideuser` | List of user ID overrides to manage. Only usable with IPA versions 4.8.7 and up. Requires "server" context. | no `rename` \| `new_name` | Rename the user object to the new name string. Only usable with `state: renamed`. | no `action` | Work on group or member level. It can be on of `member` or `group` and defaults to `group`. | no `state` | The state to ensure. It can be one of `present`, `absent` or `renamed`, default: `present`. | yes diff --git a/plugins/modules/ipagroup.py b/plugins/modules/ipagroup.py index 7251fda2..34057741 100644 --- a/plugins/modules/ipagroup.py +++ b/plugins/modules/ipagroup.py @@ -113,13 +113,14 @@ options: externalmember: description: - List of members of a trusted domain in DOM\\name or name@domain form. + Requires "server" context. required: false type: list elements: str aliases: ["ipaexternalmember", "external_member"] idoverrideuser: description: - - User ID overrides to add + - User ID overrides to add. Requires "server" context. required: false type: list elements: str @@ -188,13 +189,14 @@ options: externalmember: description: - List of members of a trusted domain in DOM\\name or name@domain form. + Requires "server" context. required: false type: list elements: str aliases: ["ipaexternalmember", "external_member"] idoverrideuser: description: - - User ID overrides to add + - User ID overrides to add. Requires "server" context. required: false type: list elements: str @@ -297,6 +299,7 @@ EXAMPLES = """ posix: yes # Create an external group and add members from a trust to it. +# Module will fail if running under 'client' context. - ipagroup: ipaadmin_password: SomeADMINpassword name: extgroup @@ -327,7 +330,8 @@ RETURN = """ from ansible.module_utils._text import to_text from ansible.module_utils.ansible_freeipa_module import \ IPAAnsibleModule, compare_args_ipa, gen_add_del_lists, \ - gen_add_list, gen_intersection_list, api_check_param + gen_add_list, gen_intersection_list, api_check_param, \ + convert_to_sid from ansible.module_utils import six if six.PY3: unicode = str @@ -562,21 +566,29 @@ def main(): # The simple solution is to switch to client context for ensuring # several groups simply if the user was not explicitly asking for # the server context no matter if mixed types are used. - context = None + context = ansible_module.params_get("ipaapi_context") if state == "present" and groups is not None and len(groups) > 1 \ and not FIX_6741_DEEPCOPY_OBJECTCLASSES: - _context = ansible_module.params_get("ipaapi_context") - if _context is None: + if context is None: context = "client" ansible_module.debug( "Switching to client context due to an unfixed issue in " "your IPA version: https://pagure.io/freeipa/issue/9349") - elif _context == "server": + elif context == "server": ansible_module.fail_json( msg="Ensuring several groups with server context is not " "supported by your IPA version: " "https://pagure.io/freeipa/issue/9349") + if ( + externalmember is not None + or idoverrideuser is not None + and context == "client" + ): + ansible_module.fail_json( + msg="Cannot use externalmember in client context." + ) + # Use groups if names is None if groups is not None: names = groups @@ -676,6 +688,23 @@ def main(): # Make sure group exists res_find = find_group(ansible_module, name) + # external members must de handled as SID + externalmember = convert_to_sid(externalmember) + + # idoverrides need to be compared through SID + idoverrideuser_sid = convert_to_sid(idoverrideuser) + res_idoverrideuser_sid = convert_to_sid( + (res_find or {}).get("member_idoverrideuser", [])) + idoverride_set = dict( + list(zip(idoverrideuser_sid or [], idoverrideuser or [])) + + list( + zip( + res_idoverrideuser_sid or [], + (res_find or {}).get("member_idoverrideuser", []) + ) + ) + ) + user_add, user_del = [], [] group_add, group_del = [], [] service_add, service_del = [], [] @@ -723,11 +752,12 @@ def main(): res_find = {} # if we just created/modified the group, update res_find - res_find.setdefault("objectclass", []) + classes = list(res_find.setdefault("objectclass", [])) if external and not is_external_group(res_find): - res_find["objectclass"].append("ipaexternalgroup") + classes.append("ipaexternalgroup") if posix and not is_posix_group(res_find): - res_find["objectclass"].append("posixgroup") + classes.append("posixgroup") + res_find["objectclass"] = classes member_args = gen_member_args( user, group, service, externalmember, idoverrideuser @@ -752,11 +782,19 @@ def main(): ) ) + # There are multiple ways to name an AD User, and any + # can be used in idoverrides, so we create the add/del + # lists based on SID, and then use the given user name + # to the idoverride. (idoverrides_add, idoverrides_del) = gen_add_del_lists( - idoverrideuser, - res_find.get("member_idoverrideuser") - ) + idoverrideuser_sid, res_idoverrideuser_sid) + idoverrides_add = [ + idoverride_set[sid] for sid in set(idoverrides_add) + ] + idoverrides_del = [ + idoverride_set[sid] for sid in set(idoverrides_del) + ] membermanager_user_add, membermanager_user_del = \ gen_add_del_lists( @@ -790,7 +828,10 @@ def main(): ) ) idoverrides_add = gen_add_list( - idoverrideuser, res_find.get("member_idoverrideuser")) + idoverrideuser_sid, res_idoverrideuser_sid) + idoverrides_add = [ + idoverride_set[sid] for sid in set(idoverrides_add) + ] membermanager_user_add = gen_add_list( membermanager_user, @@ -829,7 +870,10 @@ def main(): ) ) idoverrides_del = gen_intersection_list( - idoverrideuser, res_find.get("member_idoverrideuser")) + idoverrideuser_sid, res_idoverrideuser_sid) + idoverrides_del = [ + idoverride_set[sid] for sid in set(idoverrides_del) + ] membermanager_user_del = gen_intersection_list( membermanager_user, res_find.get("membermanager_user")) @@ -872,7 +916,7 @@ def main(): if len(externalmember_del) > 0: del_member_args["ipaexternalmember"] = \ externalmember_del - elif externalmember or external: + elif externalmember: ansible_module.fail_json( msg="Cannot add external members to a " "non-external group." diff --git a/tests/group/test_group.yml b/tests/group/test_group.yml index 8cb76946..a64b4cde 100644 --- a/tests/group/test_group.yml +++ b/tests/group/test_group.yml @@ -1,8 +1,8 @@ --- - name: Test group hosts: "{{ ipa_test_host | default('ipaserver') }}" - become: true - gather_facts: true + become: false + gather_facts: false module_defaults: ipauser: ipaadmin_password: SomeADMINpassword @@ -10,6 +10,9 @@ ipagroup: ipaadmin_password: SomeADMINpassword ipaapi_context: "{{ ipa_context | default(omit) }}" + ipaservice: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" tasks: # setup @@ -51,6 +54,16 @@ register: result failed_when: not result.changed or result.failed + - name: Ensure test service HTTP is present + ipaservice: + name: "{{ 'HTTP/' + fqdn_at_domain }}" + notify: Cleanup http service + + - name: Ensure test service LDAP is present + ipaservice: + name: "{{ 'ldap/' + fqdn_at_domain }}" + notify: Cleanup ldap service + # TESTS - name: Ensure group1 is present @@ -437,3 +450,16 @@ state: absent register: result failed_when: not result.changed or result.failed + + # ansible-lint is complaining on the use of 'when' and requiring + # the use of handlers. + handlers: + - name: Cleanup http service + ipaservice: + name: "{{ 'HTTP/' + fqdn_at_domain }}" + state: absent + + - name: Cleanup ldap service + ipaservice: + name: "{{ 'ldap/' + fqdn_at_domain }}" + state: absent diff --git a/tests/group/test_group_ad_users.yml b/tests/group/test_group_ad_users.yml new file mode 100644 index 00000000..0c84db6c --- /dev/null +++ b/tests/group/test_group_ad_users.yml @@ -0,0 +1,73 @@ +--- +- name: Test group AD external members idempotence + hosts: ipaserver + become: false + gather_facts: false + module_defaults: + ipagroup: + ipaadmin_password: SomeADMINpassword + ipaapi_context: server # external_member requires 'server' context + + vars: + ad_user: "{{ test_ad_user | default('AD\\aduser') }}" + alt_user: "{{ test_alt_user | default('aduser@ad.ipa.test') }}" + + tasks: + - name: Include tasks ../env_freeipa_facts.yml + ansible.builtin.include_tasks: ../env_freeipa_facts.yml + + - name: Ensure test group is absent. + ipagroup: + name: extgroup + state: absent + + - name: Execute group tests if trust test environment is supported + when: trust_test_is_supported | default(false) + block: + - name: Ensure external group, with AD users, is present. + ipagroup: + name: extgroup + external: true + external_member: "{{ ad_user }}" + register: result + failed_when: result.failed or not result.changed + + - name: Ensure external group, with AD users, is present, again + ipagroup: + name: extgroup + external: true + external_member: "{{ ad_user }}" + register: result + failed_when: result.failed or result.changed + + - name: Ensure external group, with alternate name AD users, is present + ipagroup: + name: extgroup + external: true + external_member: "{{ alt_user }}" + register: result + failed_when: result.failed or result.changed + + - name: Ensure external_member is absent + ipagroup: + name: extgroup + external_member: "{{ ad_user }}" + action: member + state: absent + register: result + failed_when: result.failed or not result.changed + + - name: Ensure external_member is absent, again + ipagroup: + name: extgroup + external_member: "{{ alt_user }}" + action: member + state: absent + register: result + failed_when: result.failed or result.changed + + always: + - name: Cleanup environment. + ipagroup: + name: extgroup + state: absent diff --git a/tests/group/test_group_client_context.yml b/tests/group/test_group_client_context.yml index 8d0132de..add34b6b 100644 --- a/tests/group/test_group_client_context.yml +++ b/tests/group/test_group_client_context.yml @@ -4,6 +4,9 @@ become: no gather_facts: no + vars: + ad_user: "{{ test_ad_user | default('AD\\aduser') }}" + tasks: - name: Include FreeIPA facts. ansible.builtin.include_tasks: ../env_freeipa_facts.yml @@ -18,6 +21,16 @@ failed_when: not (result.failed and result.msg is regex("No module named '*ipaserver'*")) when: ipa_host_is_client + - name: Ensuref fail if externalmember is used in client context. + ipagroup: + ipaadmin_password: SomeADMINpassword + ipaapi_context: client + name: ThisShouldNotWork + external_member: "{{ ad_user }}" + external: true + register: result + failed_when: not (result.failed and result.msg == "Cannot use externalmember in client context.") + # Import basic module tests, and execute with ipa_context set to 'client'. # If ipaclients is set, it will be executed using the client, if not, # ipaserver will be used. diff --git a/tests/group/test_group_external_members.yml b/tests/group/test_group_external_members.yml index f9214d3d..b1f9f85d 100644 --- a/tests/group/test_group_external_members.yml +++ b/tests/group/test_group_external_members.yml @@ -1,12 +1,16 @@ --- -- name: Find trust - hosts: ipaserver +- name: Test groups with external members + hosts: "{{ ipa_test_host | default('ipaserver') }}" become: false gather_facts: false module_defaults: ipagroup: ipaadmin_password: SomeADMINpassword - ipaapi_context: "{{ ipa_context | default(omit) }}" + ipaapi_context: server # external_member requires 'server' context + + vars: + ad_user: "{{ test_ad_user | default('AD\\aduser') }}" + alt_user: "{{ test_alt_user | default('aduser@ad.ipa.test') }}" tasks: @@ -24,86 +28,121 @@ when: trust_test_is_supported | default(false) block: - - name: Add nonposix group. + - name: Ensure nonposix group is present ipagroup: name: extgroup nonposix: true register: result failed_when: result.failed or not result.changed - - name: Set group to be external + - name: Ensure nonposix group is present, again + ipagroup: + name: extgroup + nonposix: true + register: result + failed_when: result.failed or result.changed + + - name: Ensure nonposix group is external ipagroup: name: extgroup external: true register: result failed_when: result.failed or not result.changed - - name: Add AD users to group + - name: Ensure nonposix group has AD users ipagroup: name: extgroup - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" register: result failed_when: result.failed or not result.changed - - name: Add AD users to group, again + - name: Ensure nonposix group has AD users, again ipagroup: name: extgroup - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" register: result failed_when: result.failed or result.changed - - name: Remove external group + - name: Ensure nonposix group is absent. ipagroup: name: extgroup state: absent register: result failed_when: result.failed or not result.changed - - name: Add nonposix, external group, with AD users. + - name: Ensure nonposix group is absent, again. + ipagroup: + name: extgroup + state: absent + register: result + failed_when: result.failed or result.changed + + - name: Ensure external group is present, with AD users. ipagroup: name: extgroup - nonposix: true external: true - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" register: result failed_when: result.failed or not result.changed - - name: Add nonposix, external group, with AD users, again. + - name: Ensure external group is present, with AD alternate users. + ipagroup: + name: extgroup + external: true + external_member: "{{ alt_user }}" + register: result + failed_when: result.failed or result.changed + + - name: Ensure external group is present, with AD users, again. ipagroup: name: extgroup - nonposix: true external: true - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" register: result failed_when: result.failed or result.changed - - name: Remove group + - name: Ensure external group is absent ipagroup: name: extgroup state: absent register: result failed_when: result.failed or not result.changed - - name: Add nonposix group. + - name: Ensure external group is absent, again + ipagroup: + name: extgroup + state: absent + register: result + failed_when: result.failed or result.changed + + - name: Ensure nonposix group is present. ipagroup: name: extgroup nonposix: true register: result failed_when: result.failed or not result.changed - - name: Set group to be external, and add users. + - name: Ensure group is external, and has AD users. ipagroup: name: extgroup external: true - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" register: result failed_when: result.failed or not result.changed - - name: Set group to be external, and add users, again. + - name: Ensure group is external, and has AD alternate users. + ipagroup: + name: extgroup + external: true + external_member: "{{ alt_user }}" + register: result + failed_when: result.failed or result.changed + + - name: Ensure group is external, and has AD users, again. ipagroup: name: extgroup external: true - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" register: result failed_when: result.failed or result.changed @@ -117,7 +156,7 @@ - name: Ensure external group members are present ipagroup: name: extgroup_members - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" action: member register: result failed_when: result.failed or not result.changed @@ -125,7 +164,7 @@ - name: Ensure external group members are present, again ipagroup: name: extgroup_members - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" action: member register: result failed_when: result.failed or result.changed @@ -133,16 +172,25 @@ - name: Ensure external group members are absent ipagroup: name: extgroup_members - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" action: member state: absent register: result failed_when: result.failed or not result.changed + - name: Ensure external group alternate members are absent + ipagroup: + name: extgroup_members + external_member: "{{ alt_user }}" + action: member + state: absent + register: result + failed_when: result.failed or result.changed + - name: Ensure external group members are absent, again ipagroup: name: extgroup_members - external_member: "AD\\Domain Users" + external_member: "{{ ad_user }}" action: member state: absent register: result diff --git a/tests/group/test_group_idoverrideuser.yml b/tests/group/test_group_idoverrideuser.yml index ce4c0bb4..2b028919 100644 --- a/tests/group/test_group_idoverrideuser.yml +++ b/tests/group/test_group_idoverrideuser.yml @@ -1,12 +1,17 @@ --- -- name: Test group +- name: Test group idoverrideuser hosts: ipaserver - become: yes - gather_facts: yes + become: false + gather_facts: false + module_defaults: + ipagroup: + ipaadmin_password: SomeADMINpassword + ipaidoverrideuser: + ipaadmin_password: SomeADMINpassword vars: - ad_user: "{{ test_ad_user | default('AD\\aduser') }}" - ad_domain: "{{ test_ad_domain | default('ad.ipa.test') }}" + ad_user: "{{ test_ad_user | default('AD\\aduser') }}" + alt_user: "{{ test_alt_user | default('aduser@ad.ipa.test') }}" tasks: - name: Include tasks ../env_freeipa_facts.yml @@ -15,38 +20,42 @@ - name: Execute tests if ipa_verison >= 4.8.7 and trust test environment is supported when: ipa_version is version("4.8.7", ">=") and trust_test_is_supported | default(false) block: - - name: Create idoverrideuser. - ansible.builtin.shell: | - kinit -c idoverride_cache admin <<< SomeADMINpassword - ipa idoverrideuser-add "Default Trust View" {{ ad_user }} - kdestroy -A -q -c idoverride_cache + - name: Ensure test idoverrideuser is present + ipaidoverrideuser: + idview: "Default Trust View" + anchor: "{{ ad_user }}" + register: result + failed_when: result.failed and "no modifications to be performed" not in result.msg - - name: Remove testing groups. + - name: Ensure test groups are absent ipagroup: - ipaadmin_password: SomeADMINpassword name: - - idovergroup + - idovergroup state: absent - - name: Add group with idoverrideuser. + - name: Ensure group with idoverrideuser is present. ipagroup: - ipaadmin_password: SomeADMINpassword name: idovergroup idoverrideuser: "{{ ad_user }}" register: result failed_when: result.failed or not result.changed - - name: Add group with idoverrideuser, again. + - name: Ensure group with idoverrideuser is present, again. ipagroup: - ipaadmin_password: SomeADMINpassword name: idovergroup idoverrideuser: "{{ ad_user }}" register: result failed_when: result.failed or result.changed - - name: Remove idoverrideuser member. + - name: Ensure group with alternative idoverrideuser is present. + ipagroup: + name: idovergroup + idoverrideuser: "{{ alt_user }}" + register: result + failed_when: result.failed or result.changed + + - name: Ensure idoverrideuser member is absent. ipagroup: - ipaadmin_password: SomeADMINpassword name: idovergroup idoverrideuser: "{{ ad_user }}" action: member @@ -54,9 +63,8 @@ register: result failed_when: result.failed or not result.changed - - name: Remove idoverrideuser member, again. + - name: Ensure idoverrideuser member is absent, again. ipagroup: - ipaadmin_password: SomeADMINpassword name: idovergroup idoverrideuser: "{{ ad_user }}" action: member @@ -64,7 +72,7 @@ register: result failed_when: result.failed or result.changed - - name: Add idoverrideuser member. + - name: Ensure idoverrideuser member is present. ipagroup: ipaadmin_password: SomeADMINpassword name: idovergroup @@ -73,7 +81,7 @@ register: result failed_when: result.failed or not result.changed - - name: Add idoverrideuser member, again. + - name: Ensure idoverrideuser member is present, again. ipagroup: ipaadmin_password: SomeADMINpassword name: idovergroup @@ -82,13 +90,7 @@ register: result failed_when: result.failed or result.changed - - name: Cleanup idoverrideuser member. - ipagroup: - ipaadmin_password: SomeADMINpassword - name: idovergroup - idoverrideuser: "{{ ad_user }}" - state: absent - + always: - name: Remove testing groups. ipagroup: ipaadmin_password: SomeADMINpassword @@ -96,10 +98,9 @@ - idovergroup state: absent - always: - name: Remove idoverrideuser. - ansible.builtin.shell: - cmd: | - kinit -c idoverride_cache admin <<< SomeADMINpassword - ipa idoverrideuser-del "Default Trust View" {{ ad_user }} - kdestroy -A -q -c idoverride_cache + ipaidoverrideuser: + idview: "Default Trust View" + anchor: "{{ ad_user }}" + continue: true + state: absent -- GitLab