IPA has a plugin which replaces a known value (ipaUniqueID=autogenerate) with a randomly generated UUID (ipaUniqueID=autogenerate). It currently executes in preoperation and internalpreoperation.
In the case of preoperation, the access controls are evaluated after the UUID plugin makes the change. This means that the access controls are useless for any attribute randomly generated by the UUID plugin. You should be able to create an ACI that restricts access to the known value only.
Here is a real world case. To allow users to create tokens, we have an ACI like this:
aci: (target = "ldap:///ipatokenuniqueid=*,cn=otp,$SUFFIX")(targetfilter = "(objectClass=ipaToken)")(version 3.0; acl "Users can create self-managed tokens"; allow (add) userattr = "ipatokenOwner#SELFDN" and userattr = "managedBy#SELFDN";)
The problem is that this permits a user to create an entry with any value. If ACIs were evaluated before the UUID plugin and not afterward, we could use this ACI:
aci: (target = "ldap:///ipatokenuniqueid=autogenerate,cn=otp, $SUFFIX")(targetfilter = "(objectClass=ipaToken)")(version 3.0; acl "Users can create self-managed tokens"; allow (add) userattr = "ipatokenOwner#SELFDN" and userattr = "managedBy#SELFDN";)
For add operations, ACIs are currently evaluated after bepreop but before betxnpreop. Unfortunately, we can't change DN/ipaTokenUniqueID in betxnpreop. For modify operations, the ACIs are currently evaluated before bepreop. One possible solution would be to move the add operation behavior to be the same as the modify operation.
Any modification of the incoming operation performed in a preop plug-in is treated as if it came from the original user by design. This means that the ACIs must be enforced on those operations. I don't think this behavior should be changed for backwards compatibility reasons (too many plug-ins may rely on this behavior).
If I understand what you are proposing, there are two parts:
1) Enforce ACIs in between preop and bepreop plug-in execution phases for ADD operations. 2) Modify the IPA UUID plug-in to be a beprop plug-in.
Do I have this correct?
This might be a possible approach to take, though we need to see what is happening in the bepreop phase for ADD operations in all existing plug-ins to be sure that we would not be allowing things that should be denied based on the current behavior. On the IPA side, there also needs to be investigation to ensure that no preop plug-ins are depending on the UUID plug-in running first, as the exection order will be changed by moding the UUID plug-in to a beprop type.
That was my proposal, yes. An alternate proposal would be to create a new phase such as beacipreop, which occurs after bepreop and after the evaluation of ACIs, but before the transaction is begun. Then UUID would be moved to beacipreop.
Obviously the question about any post-UUID dependencies needs to be considered before making the change to UUID. However, this is a FreeIPA concern. On the 389 side, we just need an appropriate hook which is before the transaction but after the ACIs.
Hi Nathaniel, Is this ticket still a blocker? Since #47924 is closed, the severity of this ticket is considered lower? Or it's independent from the issue? Thanks! --noriko
It is independent.
This bug is the only blocker left for: https://fedorahosted.org/freeipa/ticket/4456
Need to investigate the side effects from reordering the plug in hooks.
Also, fix the order inconsistency among the current backends.
Any update?
no clone (internal plugin change)
Doc: http://www.port389.org/docs/389ds/design/access-control-plugin-order-in-backend.html
pushed to 1.3.6.
I would like to see this changed also.
If we move the ACI check earlier to be consistent, we also prevent information disclosures. It saves DS processing time also.
However, the flip side is plugins that may inject / alter the ADD/MOD request. If we check too early, we may be allowing a plugin to do action on behalf of the user that we may not intend.
Saying this, the intent of the plugin may be to bypass access control, or to do exactly this ....
I think that in most cases our plugins are internally developed, and admins configure them to behave in a certain way. There are few cases where exploitation of these could happen, so I think that moving the ACI check to be soon for our ADD/MOD/DELETE is the correct answer. Additionally, these should all be keep "identical" with relation to the PRE/BEPRE/BEPRETXN location we call them. This way we don't have inconsistent behaviour or controls between types of operations (IE an ADD fails if it has some attr in it, but the same attr in a MOD works ...)
attachment 0001-Ticket-47925-Move-add-and-delete-operation-aci-check.patch
This fixes the order for add and delete. Modrdn is left "as is" because it's more complex to do, and I don't feel confident not to break it. It's also not a problem today due to the nature of modrdn vs the impact of difference in add / modify.
This passes the ACI tests as well
Ah... Sorry, William. Reviewing your patch reminded me this... http://www.port389.org/docs/389ds/design/access-control-plugin-order-in-backend.html
I did some quick investigation on this issue looong time ago and thought the acl eval orders among operations are somehow inconsistent and it'd confuse the plug-in writers. If you could update the page with your fixes, I'd appreciate it. (Please feel free to delete the page and restart a new one if it's easier.)
Also, I noticed you moved the timing of the acl check in ldbm_back_delete. I also had the question about it. Could you please take a look at "Order in ldbm_delete" in the wiki page and verify your change is the answer to my question?
As usual, please fix an indentation mismatch.
BTW, "JCM" in JCMACL is John Merrells who used to be one of the main architects of the Directory Server ;).
Metadata Update from @nkinder: - Issue assigned to firstyear - Issue set to the milestone: 1.3.6.0
<img alt="0001-Ticket-47925-Move-add-and-delete-operation-aci-check.patch" src="/389-ds-base/issue/raw/files/faee2a4749d5c45d37c9b9e2d6e6babf177deb842e4b1214ec3a1a72a7ed6a5a-0001-Ticket-47925-Move-add-and-delete-operation-aci-check.patch" />
Fixed indentation and wiki
Metadata Update from @firstyear: - Custom field reviewstatus reset - Issue close_status updated to: None
ack
Metadata Update from @mreynolds: - Custom field reviewstatus adjusted to ack
commit 3783235 To ssh://git@pagure.io/389-ds-base.git 6df837c..3783235 master -> master
Metadata Update from @firstyear: - Issue close_status updated to: fixed - Issue status updated to: Closed (was: Open)
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1256
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: fixed)
Log in to comment on this ticket.