[Freeipa-devel] [PATCH] 0137 ipasam: remove child domains before removing trust

[Martin Kosek]

On 01/20/2014 04:42 PM, Sumit Bose wrote:
> On Mon, Jan 20, 2014 at 05:18:30PM +0200, Alexander Bokovoy wrote:
>> On Mon, 20 Jan 2014, Martin Kosek wrote:
>>> On 01/20/2014 03:49 PM, Alexander Bokovoy wrote:
>>>> Hi!
>>>>
>>>> Make sure we delete child domains before removing the trust itself as
>>>> LDAP protocol does not allow removing non-leaf objects.
>>>>
>>>> This has non-obvious effect -- old code did remove cross-realm
>>>> principals and then removed trust object. However, for trusts with child
>>>> domains the trust domain object was not removed as LDAP server prevents
>>>> removing non-leaf objects. It resulted in the object still existing but
>>>> cross-realm principals missing. The trust is thus non-functioning. This
>>>> situation can be triggered with a second 'ipa trust-add' call.
>>>>
>>>> Fix the code by removing child domains first and then remove the forest
>>>> root trusted domain object.
>>>>
>>>> https://fedorahosted.org/freeipa/ticket/4126
>>>
>>> Thanks for the patch! I did not test, I am just thinking about this search:
>>>
>>> +
>>> +	rc = smbldap_search(ldap_state->smbldap_state, dn, scope, filter, NULL, 0,
>>> &result);
>>> +	TALLOC_FREE(filter);
>>> +
>>>
>>> - shouldn't you search with SCOPE_ONELEVEL given we do not dive deeper anyway?
>> No. We need to remove dn but to remove it we need to remove everything
>> under it. Thus, we don't care what is there, since whole dn
>> (cn=TRUSTNAME,cn=ad,cn=trusts,$SUFFIX) will be deleted anyway.
>>
>>> - shouldn't we search with filter "(objectclass=ipaNTTrustedDomain)" just to
>>> make sure we do not delete anything we do not want to be deleted? For example
>>> if the function gets a wrong DN, we may want to make sure we don't delete the
>>> whole DIT
>> We should delete everything under 'dn' which is cn=TRUSTNAME,cn=ad,cn=trusts,$SUFFIX
> 
> The user samba uses to bind to LDAP should not have the privileges to
> remove the whole tree. So in the worst case it will just remove all
> trusts.
> 
> I agree with Alexander that we should remove everything below the DN of
> the forest root. The original idea behind putting the other domains in
> the forest below was that be removing the sub-tree all information about
> the trust was gone (except for the idranges, but that's a different
> story).
> 
> bye,
> Sumit

Ok, makes sense. I have no conceptual objection then.

Thanks,
Martin