[PATCH] Fix server side DRSUAPI_DRS_GET_ANC handling (bug #12398)

[Andrew Bartlett]

On Wed, 2016-11-30 at 09:28 +0100, Stefan Metzmacher wrote:
> Hi Andrew,
> 
> > 
> > > 
> > > here's a patch to fix https://bugzilla.samba.org/show_bug.cgi?id=
> > > 1239
> > > 8
> > > 
> > > The problem is that the combination DRSUAPI_DRS_CRITICAL_ONLY and
> > > DRSUAPI_DRS_GET_ANC. E.g. if the administrator account was moved
> > > to an OU, samba-tool domain join DC doesn't work, as the server
> > > doesn't include all ancestors.
> > 
> > What about just fixing it client-side by requesting all the objects
> > if
> > we fail with that error?  I made our python code expose the windows
> > error codes to help with this. 
> 
> Because it's the servers job. See [MS-DRSR] 4.1.10.5.2
> GetReplChanges.
> 
> And samba-tool domain join DC --domain-critical-only needs to work
> without fetching everything. And it already does against a Windows
> dc (with the same database).

Very interesting.  I did read over that when trying to understand if we
should do similarly for extended operations, and I didn't see any
reason why we should (and added a test for the one I thought was
causing trouble).  I still can't see where critical system objects are
filtered, and non-critical system objects are included as ancestors.  

Have you found that, or is this another of the points they neglected to
clearly document?

Do you think we should cope with broken servers anyway, or just print a
pretty message and leave that to the admin?

> > 
> > > 
> > > Please review and push.
> > 
> > I think we need some tests, particularly to determine what windows
> > does
> > (if anything), and to ensure we keep the new behaviour. 
> > 
> > I certainly found that GET_ANC had no impact on the extended
> > operations, which I found surprising.  (That is why that is locked
> > down
> > in the tests). 
> 
> But you only added that for DRSUAPI_EXOP_FSMO_RID_ALLOC not for all
> others.

Sure.  That is the one I was focussed on at the time.  I don't think I
even considered REPL_OBJECT, but it will be very important to test. 

> I'll change the patch to skip it for all EXOPs.
> 
> Whould it be ok to add --domain-critical-only to 'samba-tool drs
> clone-dc-database'
> and have a test for that, while having a critical object within a non
> critical
> parent as a regression test for this.

That would be a good way to show the overall result, but we need a test
of the GetNCChanges call.  

It would probably be best in that getnc_exop.py or similar.

> I think having more detailed tests and get the 100% exact behavior as
> Windows
> is desired, but a major effort.

I want to see shown that:
 - additional objects are added to the reply
 - that they are non-critical
 - that they are added in a reasonable order (doens't need to be
exactly what windows does, but needs to be reasonable).
 - In particular that a paternity sequence of critical -> non-critical
-> critical is handled correctly.

Replication bugs bite us pretty hard, as you have seen in my changes
here, I do want them very well tested. 

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba