(ITS#6133) back-relay issues

[h.b.furuseth@usit.uio.no]

Full_Name: Hallvard B Furuseth
Version: HEAD
OS: Linux
URL: 
Submission from: (NULL) (129.240.6.233)
Submitted by: hallvard


Back-relay issues - fixing some, awaiting confirmation/discussion of others:

* The handlers for Abandon, Cancel and connection-init/destroy
  should not exist, as far as I can tell.  They forward the call to the
  underlying backend database, but that means it receives the call twice:
  Slapd calls these handlers for all databases anyway (unless one returns
  success in case of Cancel), since there's no DN on which to dispatch.
  See abandon.c:fe_op_abandon(), cancel.c:cancel_extop(), and
  backend.c:backend_connection_init()/backend_connection_destroy().

  The Cancel handler seems broken anyway.  It sets op->o_cancel =
  LDAP_CANNOT_CANCEL on the Cancel operation.  Presumably it intended
  the to-be-cancelled operation.

  I'm "#if 0"ing these out for now in case I've missed something.
  Can remove the code later.

* Fixing RB_ERR (set rs->sr_err) and RB_SEND (send if error) handling:

  relay_back_select_backend() did not obey (!(fail_mode & RB_ERR))
  except when it caught recursion.

  LDAP_SUCCESS makes no sense combined with these flags.
  relay_back_operational() could set rs->sr_err = success on failure,
  which seemed a bit excessive.  (Though it should return success.)
  Removing the RB_ERR flag.

  relay_back_chk_referrals() can send success on failure.  Except
  the function is not used, so I'm just #if 0ing and commenting it.

  This also means relay_back_select_backend() without these flags
  could just as well get SlapReply rs == NULL, which again means
  rs==NULL could function as a !RB_ERR flag.  Not doing that yet,
  in case the changes below lead to some reason not to.

* Fixing relay_back_select_backend() crash when select_backend()==NULL:
    include         servers/slapd/schema/core.schema
    database        relay
    suffix          cn=relay,cn=test
    overlay         rwm
    rwm-suffixmassage cn=nowhere
  ldapsearch -b cn=urgle,cn=relay,cn=test -s base

* back-relay can be configured to cause infinite recursion.
  This dumps core with the above search:
    include         servers/slapd/schema/core.schema
    database        relay
    subordinate
    suffix          cn=relay,cn=test
    relay           cn=relay,cn=test
    database        null
    suffix          cn=test

  There are some tests to catch that, but half-hearted ones.
  And some comments about it, like this:
	/* FIXME: this test only works if there are no overlays, so
	 * it is nearly useless; if made stricter, no nested back-relays
	 * can be instantiated... too bad. */
	if ( bd == NULL || bd == op->o_bd ) {
		return LDAP_SUCCESS;
	}
  I don't know how that's worse in this case than in other cases,
  nor what exactly how that snippet is supposed to work, so I don't
  know if the suggestion below fixes it and the test can be removed.

  The 'bd == op->o_bd' part can be removed since
  relay_back_select_backend() now returns bd==NULL when that happens:
  (bd->be_private == op->o_bd->be_private) implies (bd == op->o_bd).


  Anyway, recursion can now be properly caught with op->o_extra.  Wrap
  calls to the underlying backend (e.g. in relay_back_op) in - untested:
    	OpExtraDB oex;
	oex.oe_db = op->o_bd;
	oex.oe.oe_key = op->o_bd->be_private;
	LDAP_SLIST_INSERT_HEAD( &op->o_extra, &oex.oe, oe_next );
	...
	LDAP_SLIST_REMOVE( &op->o_extra, &oex.oe, OpExtra, oe_next );
   and check in relay_back_select_backend() with
	relay_back_info *ri = (relay_back_info *)op->o_bd->be_private;
	OpExtra *oex;
	LDAP_SLIST_FOREACH( oex, &op->o_extra, oe_next ) {
		if ( oex->oe_key == ri )
			<caught recursion, fail and return NULL>;
	}

  However, does the comment quoted above mean that it's desirable to
  allow a few rounds of recursion (a back-relay instance calling
  itself)?

  If so, define a config parameter relay-max-recursion with default=0,
  and define a struct with the OpExtra members + a counter which says
  how many rounds of recursion the operation has had so far for this
  database.  See the OpExtra comment in slap.h.