Bug 11139 - Objects referenced by a DN+Binary forward (e.g. msDS-RevealedUsers) link can't be deleted
Summary: Objects referenced by a DN+Binary forward (e.g. msDS-RevealedUsers) link can'...
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.2.0
Hardware: All All
: P5 normal (vote)
Target Milestone: 4.3
Assignee: Bob Campbell
QA Contact: Samba QA Contact
Depends on:
Blocks: 9998
  Show dependency treegraph
Reported: 2015-03-06 10:19 UTC by Stefan Metzmacher
Modified: 2017-05-15 02:54 UTC (History)
3 users (show)

See Also:

Another patch I found... (5.91 KB, text/plain)
2016-11-10 07:10 UTC, Stefan Metzmacher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2015-03-06 10:19:00 UTC
Objects referenced by a DN+Binary forward (e.g. msDS-RevealedUsers) link can't be deleted.

dsdb_dn_parse() fails with LDB_ERR_INVALID_DN_SYNTAX, because the internal code tries to delete

msDS-RevealedUsers: CN=delete1,CN=Users,DC=samba,DC=example,DC=com
from object CN=LOCALVAMPIREDC,OU=Domain Controllers,DC=samba,DC=example,DC=com.

B:96:000...000:CN=delete1,CN=Users,DC=samba,DC=example,DC=com would be
a valid value for msDS-RevealedUsers attribute.

While the client deletes CN=delete1,CN=Users,DC=samba,DC=example,DC=com.

The reason is that the backlink doesn't know about the Binary part of the dn,
while trying to delete the forward link.

Note there might be two problem related to this one for originating updates
and one for replicated updates. Replicated updates result in a failure
in dsdb_replicated_objects_commit/ldb_extended(ldb, DSDB_EXTENDED_REPLICATED_OBJECTS_OID) The error is LDB_ERR_OPERATIONS_ERROR

This is the backtrace from an originating update with:

ldbdel -H ... CN=delete1,CN=Users,DC=samba,DC=example,DC=com

#0  dsdb_dn_parse (mem_ctx=0x555556452050, ldb=0x555555760530, dn_blob=0x7fffffffca20, dn_oid=0x7ffff57fdcf2 "1.2.840.113556.1.4.903")
    at ../source4/dsdb/common/dsdb_dn.c:107
#1  0x00007fffe91cf743 in get_parsed_dns (module=0x5555558134a0, mem_ctx=0x555556410490, el=0x55555600d2f0, pdn=0x7fffffffc6f8,
    ldap_oid=0x7ffff57fdcf2 "1.2.840.113556.1.4.903", parent=0x5555568d6c30) at ../source4/dsdb/samdb/ldb_modules/repl_meta_data.c:1670
#2  0x00007fffe91d0be5 in replmd_modify_la_delete (module=0x5555558134a0, schema=0x555555e42810, msg=0x5555559d6990, el=0x55555600d2f0, old_el=0x55555615a950,
    schema_attr=0x5555572586d0, seq_num=3502, t=1425633526, msg_guid=0x7fffffffc810, parent=0x5555568d6c30)
    at ../source4/dsdb/samdb/ldb_modules/repl_meta_data.c:2094
#3  0x00007fffe91d1bc4 in replmd_modify_handle_linked_attribs (module=0x5555558134a0, msg=0x5555559d6990, seq_num=3502, t=1425633526, parent=0x5555568d6c30)
    at ../source4/dsdb/samdb/ldb_modules/repl_meta_data.c:2418
#4  0x00007fffe91d2212 in replmd_modify (module=0x5555558134a0, req=0x5555568d6c30) at ../source4/dsdb/samdb/ldb_modules/repl_meta_data.c:2554
#5  0x00007ffff676ab5a in dsdb_module_modify (module=0x5555558134a0, message=0x555556113b40, dsdb_flags=4194304, parent=0x555555f7c290)
    at ../source4/dsdb/samdb/ldb_modules/util.c:460
#6  0x00007fffe91d330b in replmd_delete_remove_link (module=0x5555558134a0, schema=0x555555e42810, dn=0x5555569bcf50, el=0x555556038ce0, sa=0x555557258280,
    parent=0x555555f7c290) at ../source4/dsdb/samdb/ldb_modules/repl_meta_data.c:2947
#7  0x00007fffe91d445e in replmd_delete_internals (module=0x5555558134a0, req=0x555555f7c290, re_delete=false)
    at ../source4/dsdb/samdb/ldb_modules/repl_meta_data.c:3304
#8  0x00007fffe91d4af9 in replmd_delete (module=0x5555558134a0, req=0x555555f7c290) at ../source4/dsdb/samdb/ldb_modules/repl_meta_data.c:3444
Comment 1 Stefan Metzmacher 2016-06-03 10:08:21 UTC
Andrew, is this the fix?
Comment 2 Stefan Metzmacher 2016-06-03 10:09:04 UTC
(In reply to Stefan Metzmacher from comment #1)
Comment 3 Stefan Metzmacher 2016-06-03 10:15:09 UTC
(In reply to Stefan Metzmacher from comment #2)

Maybe together with
Comment 4 Stefan Metzmacher 2016-06-03 10:18:59 UTC
(In reply to Stefan Metzmacher from comment #3)

Ok, they seem to be basically the same, but dereference

schema_attr at different levels.

I guess I'd prefer Andrew's version that passes the full
Comment 5 Andrew Bartlett 2016-06-03 10:20:49 UTC
(In reply to Stefan Metzmacher from comment #4)
It is the fix, but it broke make test and so we dropped it so that we could get whatever else we were doing at the time into master.
Comment 6 Stefan Metzmacher 2016-11-10 07:10:35 UTC
Created attachment 12638 [details]
Another patch I found...

Hi Andrew,

I some where found the attached patch from you in one of my branches,
also pointing to this bug report.

Can we bring this to an end and push something to master?
Comment 7 Andrew Bartlett 2016-11-10 07:52:40 UTC
(In reply to Stefan Metzmacher from comment #6)

I'm happy with that patch, if you are happy that we have enough tests to cover it.  I chatted with Garming about it recently, and our only blocker was that we didn't know if we had sufficient tests.  Apparently it passes an autobuild these days.
Comment 8 Stefan Metzmacher 2016-12-23 05:51:30 UTC
(In reply to Andrew Bartlett from comment #7)

Ok, the following went into master now:

commit 53377917beeb18553904dc53b227ecf1745a5d1f
Author: Andrew Bartlett <abartlet@samba.org>
Date:   Wed May 20 11:06:22 2015 +0200

    dsdb: Parse linked attributes using their DN+Binary or DN+String syntax, if needed
    Signed-off-by: Andrew Bartlett <abartlet@samba.org>
    Reviewed-by: Garming Sam <garming@catalyst.net.nz>

But we don't have any tests that verify the new behaviour,
so I'm unsure if the bug is really fully fixed.
Or we also need https://bugzilla.samba.org/attachment.cgi?id=12638

I think we need a test that adds a DN+* link and replicates it
removes it and replicates the deletion.
Comment 9 Andrew Bartlett 2016-12-23 06:45:43 UTC
(In reply to Stefan Metzmacher from comment #8)

I've clearly not been paying enough attention, I agree both patches need to land. and I mixed them up in my head. 

Bob was working to try and find the regression in the patch, but couldn't, because we mixed up the patches.  

Bob:  Can you try and do the tests metze has indicated, plus check what the original autobuild failure was?  (As before, starting with the old branch if we can't reproduce it on master).
Comment 10 Bob Campbell 2016-12-28 00:05:51 UTC
This new test:
Fails before and after both of Andrew's patches in dsdb_dn.c:107 as expected.

I'm running autobuilds before & after Andrew's first patch to see what the initial problem was and whether or not it fixed anything.
Comment 11 Bob Campbell 2016-12-28 03:04:20 UTC
(In reply to Bob Campbell from comment #10)

That test was incorrect; here is an updated patch where we delete the backlink:
This fails before the two patches and passes afterwards.