What about targetfilter = "( uid )"? Shouldn't that be valid too?

What about targetfilter = "( uid )"? Shouldn't that be valid too?

That is technically invalid of course, and probably still goes undetected, but this heap underrun issue only occurs with "==", as that's how we separate the filter from the aci keyword. There are more details in the bugzilla, but for some reason all the comments are private (I think they thought this could of have been a security issue).

Ahhh okay, all good to me then.

Lot's of ACL tests are failing because now we're enforcing proper syntax: https://fedorapeople.org/groups/389ds/ci/nightly/2020/04/30/report-389-ds-base-1.4.4.1-20200429gitc7da66e.fc31.x86_64.html

We can fix the tests, of course. But this change has a big impact on existing installations that have invalid acis already. Existing invalid acis will be ignored:

[30/Apr/2020:07:37:54.864068234 +0000] - ERR - NSACLPlugin - __aclp__init_targetattr - targetattr has an invalid value (targetattr=*)
[30/Apr/2020:07:37:54.868470074 +0000] - ERR - NSACLPlugin - acllist_insert_aci_needsLock_ext - ACL PARSE ERR(rv=-5): (targetattr=*
[30/Apr/2020:07:37:54.872568916 +0000] - ERR - NSACLPlugin - __aclinit_handler - This  ((targetattr=*)(targetfilter="(objectClass=inetOrgPerson)")(version 3.0; acl "Test aci"; allow (read, search, compare)(userdn="ldap:///anyone");)) ACL will not be considered for evaluation because of syntax errors.

That also includes FreeIPA, as they have targetattr=* or unquoted values in a quite few places.

I suggest to roll back this change for now, sync with FreeIPA team so they can fix their acis.
We also need to provide a smooth upgrade path (healthcheck tool + fixup task?) and have a release note.

@vashirov - Do the aci's that use "targetattr=*" work correctly? I can change the fix to just reduce the multiple equal signs into a single one - that would also resolve this.

DS complains about them too and they get ignored:
https://pagure.io/freeipa/issue/8301
https://pagure.io/dogtagpki/issue/3173

I filed PRs for freeipa and dogtag to address these issues:
https://github.com/freeipa/freeipa/pull/4623
https://github.com/dogtagpki/pki/pull/397

I think that in the end we should enforce the correct syntax. But we also need to help users to identify incorrect ACIs (via https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/aci.py#_62 for example) and fix them before we do the enforcement.

Plenty of history says people won't read the release notes, so I think maybe we should check for a leading " or * instead?

Plenty of history says people won't read the release notes, so I think maybe we should check for a leading " or * instead?

@firstyear - I think you meant to update a different PR :-)

Anyway, that's not good enough because you can use a single attribute: targetattr=cn In code we explicitly state you don't need quotes for single values, although in the docs we ALWAYS use quotes - so that is the syntax we are striving for, but in order to not break any existing deployments we need to allow:

• targetattr=*
• targetattr=ABCabc

The next plan is add the ACI checks into healthcheck to find these ACI's and let customers know they are "invalid" and should be changed. Then at a later date/release we can flip the switch to enforce proper syntax.

No correct PR :) https://pagure.io/389-ds-base/pull-request/51055#comment-118222 this comment viktor mentions to release note this breaking change.

I think having this in the healthcheck is a better place than just release notes, and perhaps even in an upgrade/migration guide we require that people run a healthcehck before they start?

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4108

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.