dbcheck: Do not regard old one-way-links as errors

Samba does not maintain one way links when the target is deleted or renamed
so do not fail dbcheck because of such links, but allow them to be updated.

This matters because administrators and make test expect that normal Samba
operation do NOT cause the database to become corrupt, and any error from
dbcheck tends to trigger alarms (or test failures).

If an object pointed at by a one way link is renamed or deleted in normal
operations (such as intersiteTopologyGenerator pointing at a demoted DC),
or make test, then this could trigger.

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Garming Sam <garming@catalyst.net.nz>
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12577

2 files changed, 43 insertions, 10 deletions

diff --git a/python/samba/dbchecker.py b/python/samba/dbchecker.py
index 3fcfbc05639..22819dedcdf 100644
--- a/python/samba/dbchecker.py
+++ b/python/samba/dbchecker.py
@@ -59,6 +59,7 @@ class dbcheck(object):
         self.fix_all_string_dn_component_mismatch = False
         self.fix_all_GUID_dn_component_mismatch = False
         self.fix_all_SID_dn_component_mismatch = False
+        self.fix_all_old_dn_string_component_mismatch = False
         self.fix_all_metadata = False
         self.fix_time_metadata = False
         self.fix_undead_linked_attributes = False
@@ -574,6 +575,23 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base)))
                           "Failed to fix %s on attribute %s" % (errstr, attrname)):
             self.report("Fixed %s on attribute %s" % (errstr, attrname))
 
+    def err_dn_string_component_old(self, dn, attrname, val, dsdb_dn, correct_dn):
+        """handle a DN string being incorrect"""
+        self.report("NOTE: old (due to rename or delete) DN string component for %s in object %s - %s" % (attrname, dn, val))
+        dsdb_dn.dn = correct_dn
+
+        if not self.confirm_all('Change DN to %s?' % str(dsdb_dn),
+                                'fix_all_old_dn_string_component_mismatch'):
+            self.report("Not fixing old string component")
+            return
+        m = ldb.Message()
+        m.dn = dn
+        m['old_value'] = ldb.MessageElement(val, ldb.FLAG_MOD_DELETE, attrname)
+        m['new_value'] = ldb.MessageElement(str(dsdb_dn), ldb.FLAG_MOD_ADD, attrname)
+        if self.do_modify(m, ["show_recycled:1"],
+                          "Failed to fix old DN string on attribute %s" % (attrname)):
+            self.report("Fixed old DN string on attribute %s" % (attrname))
+
     def err_dn_component_target_mismatch(self, dn, attrname, val, dsdb_dn, correct_dn, mismatch_type):
         """handle a DN string being incorrect"""
         self.report("ERROR: incorrect DN %s component for %s in object %s - %s" % (mismatch_type, attrname, dn, val))
@@ -914,12 +932,16 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base)))
                 if rmd_flags & 1:
                     continue
 
-            # check the DN matches in string form
-            if str(res[0].dn) != str(dsdb_dn.dn):
-                error_count += 1
-                self.err_dn_component_target_mismatch(obj.dn, attrname, val, dsdb_dn,
-                                                      res[0].dn, "string")
-                continue
+            # assert the DN matches in string form, where a reverse
+            # link exists, otherwise (below) offer to fix it as a non-error.
+            # The string form is essentially only kept for forensics,
+            # as we always re-resolve by GUID in normal operations.
+            if reverse_link_name is not None:
+                if str(res[0].dn) != str(dsdb_dn.dn):
+                    error_count += 1
+                    self.err_dn_component_target_mismatch(obj.dn, attrname, val, dsdb_dn,
+                                                          res[0].dn, "string")
+                    continue
 
             if res[0].dn.get_extended_component("GUID") != dsdb_dn.dn.get_extended_component("GUID"):
                 error_count += 1
@@ -933,9 +955,18 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base)))
                                                       res[0].dn, "SID")
                 continue
 
+            # Now we have checked the GUID and SID, offer to fix old
+            # DN strings as a non-error (for forward links with no
+            # backlink).  Samba does not maintain this string
+            # otherwise, so we don't increment error_count.
+            if reverse_link_name is None:
+                if str(res[0].dn) != str(dsdb_dn.dn):
+                    self.err_dn_string_component_old(obj.dn, attrname, val, dsdb_dn,
+                                                     res[0].dn)
+                continue
 
-            # check the reverse_link is correct if there should be one
-            if reverse_link_name is not None:
+            else:
+                # check the reverse_link is correct if there should be one
                 match_count = 0
                 if reverse_link_name in res[0]:
                     for v in res[0][reverse_link_name]:
diff --git a/testprogs/blackbox/renamedc.sh b/testprogs/blackbox/renamedc.sh
index 3eb5817c418..7767d9d68ef 100755
--- a/testprogs/blackbox/renamedc.sh
+++ b/testprogs/blackbox/renamedc.sh
@@ -65,8 +65,10 @@ testrenamedc2() {
 }
 
 dbcheck_fix() {
+        # Unlike most calls to dbcheck --fix, this will not trigger an error, as
+        # we do not flag an error count for this old DN string case.
 	$BINDIR/samba-tool dbcheck --cross-ncs -s $PREFIX/renamedc_test/etc/smb.conf --fix \
-		--quiet --yes fix_all_string_dn_component_mismatch \
+		--quiet --yes fix_all_old_dn_string_component_mismatch \
 		--attrs="fsmoRoleOwner interSiteTopologyGenerator msDS-NC-Replica-Locations"
 }
 
@@ -83,7 +85,7 @@ testit "confirmrenamedc_sAMAccountName" confirmrenamedc_sAMAccountName || failed
 testit "confirmrenamedc_dNSHostName" confirmrenamedc_dNSHostName || failed=`expr $failed + 1`
 testit "confirmrenamedc_rootdse_dnsHostName" confirmrenamedc_rootdse_dnsHostName || failed=`expr $failed + 1`
 testit "confirmrenamedc_rootdse_dsServiceName" confirmrenamedc_rootdse_dsServiceName || failed=`expr $failed + 1`
-testit_expect_failure "dbcheck_fix" dbcheck_fix || failed=`expr $failed + 1`
+testit "dbcheck_fix" dbcheck_fix || failed=`expr $failed + 1`
 testit "dbcheck" dbcheck || failed=`expr $failed + 1`
 testit "renamedc2" testrenamedc2 || failed=`expr $failed + 1`