[PATCH] s4:drs Create connection object (nTDSConnection)

[tridge at samba.org]

Hi Cristian,

Thanks for the patch!

The problem you are getting with the fromServer attribute is because
you are treating it as a binary blob, not a string. When you're trying
to work out how to deal with a particular attribute type it can be
useful to look at how it is dealt with elsewhere in Samba. So for
example you could run this:

  git grep ldb_msg_add | grep dn

and you'll see lots of places that add a dn to a message. You'll see
that they treat it as a string. 

It is also arguarble that we should add a new helper function in
ldb_msg.c called ldb_msg_add_dn() which takes a 'struct ldb_dn *'
directly. It would look something like this:

int ldb_msg_add_dn(struct ldb_message *msg, 
		   const char *attr_name, struct ldb_dn *dn)
{
	return ldb_msg_add_string(msg, attr_name, ldb_dn_get_linearized(dn));
}

If you do decide to add this helper, then you could send a separate
patch that makes the other places in Samba that add DNs to also use
this function.

After you fix the above in your patch you'll find that it will create
the object successfully. There are some other issues with the patch
that you should fix up however.

  1) Each time the kcc runs it re-runs this function. That means it
  keeps creating new connection objects, whereas we only want one
  connection object per DC that we are replicating with. We also need
  to remove connection objects from DCs that we are no longer
  replicating with (ie. if the nTDSDSA object of that DC is gone). So
  you'll need to do a LDB_SCOPE_ONELEVEL search under our nTDSDSA
  object to find all the existing connection objects we have, and then
  only add missing ones, plus delete ones that should not be there. So
  you'll probably need one function that works out a list of what
  connections we should have, then a separate function that applies
  that, adding/deleting as needed.

  2) you need a lot more error checking. There are lots of functions
  in kccsrv_create_connection() that could fail, and you need to
  report an appropriate error if they do fail. This is especially the
  case for any calls that do searches (such as
  dsdb_find_dn_by_guid()). If in doubt, please err on the side of
  checking error returns rather than not. When something fails we need
  to know what went wrong by looking at the logs.

  3) you should probably add a higher level debug message (maybe level
  2) when you create a new object or delete one, saying what object
  you are creating, and what server it is for. The admin can then see
  what happened by looking at the logs.

  4) your function kccsrv_create_connection() returns an error, but
  the caller never looks at it. You should either make the function
  return void, or put the return code into a variable in the caller
  and print an appropriate message if it fails.

  5) Why are you adding an invocationId ? I don't see one on
  nTDSConnection objects on windows, and I don't see it as an
  attribute for this class in the schema. (the invocationId you are
  adding also looks wrong - GUIDs need to be added as NDR objects).

Once you fix the fromServer problem, you should also now compare the
resulting objects with what windows produces and look into any
other discrepancies. Also make sure you test the case when a
connection should be deleted.

While looking at your patch I also noticed that the invocationId is
not displaying as a nicely formatted GUID in ldbsearch. 

 > By the way, I'm not sure if I should send this patch as a top-level e-mail
 > (like this one) or as a reply to the previous e-mail discussing this
 > implementation.

It is good to do it as a new mail, with a [PATCH] prefix just like
you've done. Thanks!

Cheers, Tridge