[PATCH] DsReplGetInfo() for DS_REPL_INFO_REPSTO infoType

[tridge at samba.org]

Hi Erick,

I've had a look over your patch, and there are a few cosmetic things
that I'd like fixed. 

Your patch uses "goto DONE;" in quite a few places inappropriately. We
do often use a "goto failed;" style in Samba, but we use it when the
failure path needs some common cleanup code that would otherwise have
to be reproduced for each failure case. For this patch, your use of:

  status = WERR_XXX_YYY;
  goto DONE;

is exactly equivalent to:

  return WERR_XXX_YYY;

so the simpler 'return' method is more appropriate, and easier to
read.

I'd also prefer you didn't have the if() statement inside the switch
for DRSUAPI_DS_REPLICA_INFO_NEIGHBORS and
DRSUAPI_DS_REPLICA_INFO_NEIGHBORS02. If you want the common pre-amble
code, then it would be better to put that common code inside helper
functions, and keep the switch statement as a very simple 1 or 2 lines
per switch value.

Please also have a short comment block before each function explaining
what it does, and giving a pointer into the WSPP docs where
appropriate.

I also wonder in kccdrs_replica_get_info_repsto() why you carefully
build a linked list, and then convert it to an array? Why not build it
as an array with talloc_realloc() directly? (the list is likely to be
quite short, so the cost of the reallocs will be low).


 > On the torture test for DsGetReplInfo(), torture/rpc/dsgetinfo.c,
 > the request for the DRSUAPI_DS_REPLICA_INFO_OBJ_METADATA infoType returns
 > WERR_UNKNOWN_LEVEL (the right value, because samba doesn't that infoType
 > yet),
 > but the test stops at this infoType (it says expected WERR_OK), and
 > doesn't run the other ones. How can I make it continue and test the other
 > infoTypes ?

If you look at the test_getinfo() code, you'll see that it tries to
cope with this by looking for DCERPC_FAULT_INVALID_TAG and just
reporting that the server doesn't support that info level if it sees
that error code. The reason that doesn't work with Samba is that Samba
does have the info level in our IDL file, so we pass the NDR parsing
layer, and thus do not produce a RPC fault.

So I'd suggest that you modify the test_getinfo() code to look for
WERR_INVALID_LEVEL, and if it gets that return then just print a
warning and increment a counter of failed levels. Then return false at
the end of the function if that counter is != 0.

 > I've seen that someone (maybe you, I'm not right) modified
 > dcesrv_drsuapi_DsReplicaGetInfo()
 > on dcesrv_drsuapi.c such that the user level must be at least
 > SECURITY_ADMINISTRATOR, is that right?

that's right

 > If so, that must be the same required level for other calls, for example
 > dcesrv_drsuapi_DsReplicaSync() and dcesrv_drsuapi_DsExecuteKCC(), right?

DsReplicaSync() already checked for authorization by calling
drs_security_level_check() which checks that the caller is a domain
controller. DsExecuteKCC() makes the same check.

It is possible that we should change these to allow administrators to
make these calls. Can you test against Windows and see if
administrators can make all these calls? If they can, then we should
add a common function that checks for SECURITY_ADMINISTRATOR and use
that common function in these various drs functions.

 > I'm having a problem running "repadmin <op> <samba-dc>" over my samba box
 > where op is "/kcc" or "/showrepl" or "/showrepl /repsto" or "/syncall".
 > Repadmin returns "failed with status 8453 (0x2105): Replication access was
 > denied." and samba prints "rpc_server/drsuapi/dcesrv_drsuapi.c:753:
 > Administrator access required for DsReplicaGetInfo" 

yes, this is a bug in the groups handling in Samba. Andrew Bartlett is
looking into it (or he will when he is back from holiday!). Meanwhile,
you can start Samba like this:

  sudo bin/samba -i -M single --option=drs:disable_sec_check=true

and that will disable those checks (that is why I have the
lp_parm_bool() check in dcesrv_drsuapi_DsReplicaGetInfo().

Cheers, Tridge