#47925 Need a way for plugin to modify the request during ADD op. after the ACIs are evaluated
Closed: wontfix 7 years ago Opened 9 years ago by npmccallum.

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

Need to investigate the side effects from reordering the plug in hooks.

Also, fix the order inconsistency among the current backends.

no clone (internal plugin change)

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 ...)

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

7 years ago

Metadata Update from @firstyear:
- Custom field reviewstatus reset
- Issue close_status updated to: None

7 years ago

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to ack

7 years ago

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)

7 years ago

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.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: fixed)

3 years ago

Login to comment on this ticket.

Metadata