Add module to modify ACL's on Active Directory objects #72
Conversation
Docs Build 📝Thank you for contribution!✨ The docsite for this PR is available for download as an artifact from this run: You can compare to the docs for the File changes:
Click to see the diff comparison.NOTE: only file modifications are shown here. New and deleted files are excluded. diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/index_module.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/index_module.html
index 905b056..49bd556 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/index_module.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/index_module.html
@@ -124,6 +124,7 @@
<section id="microsoft-ad">
<h2>microsoft.ad<a class="headerlink" href="#microsoft-ad" title="Link to this heading"></a></h2>
<ul class="simple">
+<li><p><a class="reference internal" href="microsoft/ad/acl_module.html#ansible-collections-microsoft-ad-acl-module"><span class="std std-ref">microsoft.ad.acl</span></a> – Used to set ACL’s on objects in an Active Directory.</p></li>
<li><p><a class="reference internal" href="microsoft/ad/computer_module.html#ansible-collections-microsoft-ad-computer-module"><span class="std std-ref">microsoft.ad.computer</span></a> – Manage Active Directory computer objects</p></li>
<li><p><a class="reference internal" href="microsoft/ad/debug_ldap_client_module.html#ansible-collections-microsoft-ad-debug-ldap-client-module"><span class="std std-ref">microsoft.ad.debug_ldap_client</span></a> – Get host information for debugging LDAP connections</p></li>
<li><p><a class="reference internal" href="microsoft/ad/domain_module.html#ansible-collections-microsoft-ad-domain-module"><span class="std std-ref">microsoft.ad.domain</span></a> – Ensures the existence of a Windows domain</p></li>
diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/computer_module.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/computer_module.html
index 4fd060a..decf642 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/computer_module.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/computer_module.html
@@ -25,7 +25,7 @@
<script src="../../../_static/js/theme.js"></script>
<link rel="search" title="Search" href="../../../search.html" />
<link rel="next" title="microsoft.ad.debug_ldap_client module – Get host information for debugging LDAP connections" href="debug_ldap_client_module.html" />
- <link rel="prev" title="Migration guide" href="docsite/guide_migration.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
+ <link rel="prev" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." href="acl_module.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
@@ -724,7 +724,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-comput
<footer><div class="rst-footer-buttons" role="navigation" aria-label="Footer">
- <a href="docsite/guide_migration.html" class="btn btn-neutral float-left" title="Migration guide" accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
+ <a href="acl_module.html" class="btn btn-neutral float-left" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
<a href="debug_ldap_client_module.html" class="btn btn-neutral float-right" title="microsoft.ad.debug_ldap_client module – Get host information for debugging LDAP connections" accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
</div>
diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/docsite/guide_migration.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/docsite/guide_migration.html
index f420d7d..520aceb 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/docsite/guide_migration.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/docsite/guide_migration.html
@@ -23,7 +23,7 @@
<script src="../../../../_static/sphinx_highlight.js?v=dc90522c"></script>
<script src="../../../../_static/js/theme.js"></script>
<link rel="search" title="Search" href="../../../../search.html" />
- <link rel="next" title="microsoft.ad.computer module – Manage Active Directory computer objects" href="../computer_module.html" />
+ <link rel="next" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." href="../acl_module.html" />
<link rel="prev" title="Setting list option values guide" href="guide_list_values.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
@@ -281,7 +281,7 @@
<footer><div class="rst-footer-buttons" role="navigation" aria-label="Footer">
<a href="guide_list_values.html" class="btn btn-neutral float-left" title="Setting list option values guide" accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
- <a href="../computer_module.html" class="btn btn-neutral float-right" title="microsoft.ad.computer module – Manage Active Directory computer objects" accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
+ <a href="../acl_module.html" class="btn btn-neutral float-right" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
</div>
<hr/>
diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/index.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/index.html
index d00f9e4..b486036 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/index.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/index.html
@@ -189,6 +189,7 @@
<section id="modules">
<h3>Modules<a class="headerlink" href="#modules" title="Link to this heading"></a></h3>
<ul class="simple">
+<li><p><a class="reference internal" href="acl_module.html#ansible-collections-microsoft-ad-acl-module"><span class="std std-ref">acl module</span></a> – Used to set ACL’s on objects in an Active Directory.</p></li>
<li><p><a class="reference internal" href="computer_module.html#ansible-collections-microsoft-ad-computer-module"><span class="std std-ref">computer module</span></a> – Manage Active Directory computer objects</p></li>
<li><p><a class="reference internal" href="debug_ldap_client_module.html#ansible-collections-microsoft-ad-debug-ldap-client-module"><span class="std std-ref">debug_ldap_client module</span></a> – Get host information for debugging LDAP connections</p></li>
<li><p><a class="reference internal" href="domain_module.html#ansible-collections-microsoft-ad-domain-module"><span class="std std-ref">domain module</span></a> – Ensures the existence of a Windows domain</p></li>
|
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 47s |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 00s |
|
Thanks for the PR, I'll be traveling next week so won't have too much time to look into this. One thing I would mention is that this might be a good option for setting single rules but we might need to look into making things more efficient by allowing multiple rules to be set. The UX behind such a module is going to be difficult to work out here unfortunately. |
|
Hi, I also considered making both rights and rights_attr arrays and then generate rules for all permutations, another alternative is adding a list with rights/rights_attr and adding all these. One thing to consider is that rights_attr is optional, just giving a rights will be set on all attributes unless rights_attr is specified. /Mikael |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 9m 36s |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 38s |
|
Hi, I did some minor cleanups, the rights argument is handled by the enum-type so it can already by multiple values. I also added the inherited type so it is possible to let a user/group get access to children of a specific type, see the examples in the python-file /Mikael |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 21s |
|
Sorry for not getting back to you on this beforehand, I've been tied up in some unrelated work to this collection and have unfortunately left this on the backlog. The changes you have here seem like a decent module but I'm not sure if it's the direction to go in. I don't want to leave it open any longer without me actually replying to the hard work you've put in here and list the reasons why I haven't done a full review merge yet. Looking at the code here it definitely resembles how the win_acl module works but I've been apprehensive about replicating that module for a few reasons:
It is also missing things like check mode, diff support, and tests which is something I'm very keen on having on all the modules in this collection. This is not a complaint against the code you've written, just something that would need to be added if we choose to continue down the option you are proposing here. I keep on coming back to the idea of a - microsoft.ad.security_descriptor:
# Selects the object to manage
name: obj
identity: obj
path: ...
# SD bits
owner: sid or account name
group: sid or account name
dacl:
add:
- identity: sid or account name
rights: list of numbers or known string flags
type: allow|deny
object_type: guid or name
inheritance_type: all|children|descendents|none|self_and_children
inherited_object_type: guid or name
# Defines how to compare to existing rules, exact is exact while
# rights must have the rights set in the bitmask
compare_rule: exact|rights
remove: ...
set: ...
inheritance: absent|present
# When inheritance: absent, converts to explicit rules, when present
# dedups explicit rules if they match newly added inherited ones
inheritance_reorganize: true|false
sacl:
add:
- identity: sid or account name
rights: list of numbers or known string flags
type: none|failure|success
object_type: guid or name
inheritance_type: all|children|descendents|none|self_and_children
inherited_object_type: guid or name
compare_rule: exact|rights
remove: ...
set: ...
inheritance: absent|present
inheritance_perserve: true|false
label:
flags: no_write_up|no_read_up|no_execute_up
value: untrusted|low|medium|high|system|protected_processThere's also nothing stopping having this as a more efficient module for doing a one big definition vs also making a Ultimately I think I am leaning towards having the following separate modules to match each of the entries in the definition above
I could even go a step further an make those modules more of a module util and still do the big I am curious as to your thoughts on all this and what you think about the proposal. Knowing how someone actually would use this in the real world will definitely help shape how it all works. |
|
I like your idea and I think both the security_descriptor and the separate modules will work, the problem is the rights system is complex, even more so in an Active Directory, and I think it will be hard to "simplyfy" the modules to much, the modules needs to be complex to handle complex rules. The reason I needed to modify the acl's was to have an OU with a group and a number of normal users and the users should be able to create additional users in the OU and also add them to the group, for this one rule is needed on the OU to allow for new users to be created, one rule for modify users in the OU and one rule for modifying the group. These are the examples in the pull request. For this I think I would prefer the separate modules solution, it would be three microsoft.ad.sd_dacl rules since this is what I would change. I like that you have the set-option in all modules, I like to set the correct value and then be able to verify that nothing new has been added. |
|
Yeah, I concur with most of @jborean93 's thoughts here- the functionality and UX requirements we're trying to enforce for new modules in "officially supported" collections are a bit more onerous. Speaking from our own experiences trying to do so, it's often difficult to retrofit things like multiple resource support and "add vs replace" to existing modules without a bunch of complex heuristics or confusing the UX with weird mode args. The results are a lot better when those things are designed in from the beginning. The fact that it's somewhat difficult to canonically "key" an ACE and definitively say "yes, this ACE/ACL is logically equivalent to that one" makes it even gnarlier- I remember pulling my own hair out trying to write a basic idempotent ACL manipulation of a random system object for an installer ~ 20 years ago. I also agree that one invocation per ACE is probably not the right way to go- for DACLs anyway, the resource granularity should be (at minimum) the DACL, not the individual ACEs. I'm a bit torn on the separate modules thing; IME owner and dacl are by far the most commonly used things, so focusing on those is probably most important. Since the underlying resource really is just the SD, and the DACL is probably what most folks are after, I'm not sure it'd be so bad to just have one @jborean93 were your concerns about a single SD module mostly related to UX/docs or implementation? |
The latter. Trying to document the somewhat nested structures and the values will get quite complex, especially once we start adding in support for SACLs. At least with smaller modules the documentation would be focused specifically on that one component. Regardless of whether this was one giant module vs separate ones for each SD component the implementation will most likely have to be the same. The owner/group ones are simple but as you said trying to determine if an ACE is present in an ACL will be where the complexity lies. |
|
Fair enough, and after thinking about it a little more, I suppose since the SD itself isn't named, you're never going to "create one from scratch", you're just adjusting properties on an existing object. I can definitely see the attraction to removing a layer of nesting, since our docs and argspec stuff for complex nested structures has improved, but is still "worse than flat". That said, I did have another minor twist on the UX you described above that gives you the same level of nesting as discrete modules but still all in one module: top-level attr keys for owner/group/label, but then I don't know why the multiple tiny modules thing feels icky for this case, but it kinda does. I think maybe it's the thought of the "creating an entirely new OU/object" case possibly requiring ~5 module invocations instead of 1-2. 🤷 |
I'm not sure if that would really help the situation too much. The docs will still be quite large as the entries for
Yea I personally prefer the idea of just 1 module for this but was ultimately planning on having both from a documentation and ease of discoverability for the user. They can all reference each other and ultimately the code will be using a common module_util so we don't have to worry about code duplication here. |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 38s |
|
Personally, I would prefer having one "big" module which can basically do everything that IMO, starting with add/remove only handling exact matches would be perfectly fine. Adding options to allow fuzzier matching is still possible later on. @jborean93 I am not quite sure about the meaning of I have one more feature idea that would be really helpful: Having an option to ignore the default objectClass ACE (from defaultsecuritydescriptor) when using In summary, I would very much appreciate a module to manage AD ACLs! :) |
|
I have just opened an issue in the A feature I mentioned there, which also seems relevant to this AD ACL module, is having some kind of recursive mode, to realiably remove all non-inherited/non-managed/non-default ACEs from child objects. |
@Yannik please see #39 (comment) for a temporary solution. Hope it helps. |
SUMMARY
A new module to modify ACL's on objects in an Active Directory
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
I need to set permission on a group to let the Managed By group be able to add/remove members, this also ticks the "Manager can update membership list" for the group.