[Freeipa-devel] [PATCH] 0118 add support for subdomains

[Jan Cholasta]

On 20.9.2013 09:36, Alexander Bokovoy wrote:
> On Fri, 20 Sep 2013, Jan Cholasta wrote:
>> On 19.9.2013 21:08, Alexander Bokovoy wrote:
>>> Hi!
>>>
>>> Attached patch adds IPA CLI to manage trust subdomains.
>>>
>>> ipa trust-domain-fetch <trust>   -- fetch list of subdomains from AD
>>> side and add new ones to IPA
>>> ipa trust-domain-find <trust>    -- show all available subdomains ipa
>>> trust-domain-del <trust> <domain> -- remove subdomain from IPA view
>>> about <trust>
>>> ipa trust-domain-mod <trust> <domain> -- modify subdomain parameters
>>> (work in progress)
>>>
>>> IPA KDC needs also information for authentication paths to subdomains in
>>> case they are not hierarchical under AD forest trust root. This
>>> information is managed via capaths section in krb5.conf. SSSD should be
>>> able to generate it once ticket
>>> https://fedorahosted.org/sssd/ticket/2093 is resolved.
>>>
>>> part of https://fedorahosted.org/freeipa/ticket/3909
>>>
>>> The patch implements some dark magic to get around IPA framework
>>> limitations:
>>>
>>>  -- CLI commands belong to 'trust' family but operate on 'subdomain'
>>> object
>>>  -- 'subdomain' objects are stored under trust container, thus making
>>>     container_dn dependent on a particular trust:
>>>      cn=<subdomain>,cn=<trust>,cn=ad,cn=trusts,$SUFFIX
>>>
>>> The latter is a design decision since our KDC driver loads all objects
>>> with objectclass=ipaNTTrustedDomain from cn=ad,cn=trusts,$SUFFIX using
>>> subtree scope. With this design no changes were needed in ipa-kdb at all
>>> to support subdomains.
>>>
>>
>> NACK, this patch breaks several conventions we use in the framework:
> Have you read the reasoning first?

Yes.

>
>> 1) The object is named "subdomain", but the commands are named
>> "trust_domain_*". Please use the object name as the base for command
>> names. I would suggest renaming the object to "trustdomain", as the
>> framework does not allow underscores in object names, and "subdomain"
>> sounds a little bit too generic.
> subdomains as objects are not visible to users. Users cannot operate on
> subdomains themselves. I do not want to give you impression we are
> having separate trust and domains. Instead, we deal with trusts and
> subdomains are simply internal components of it.

They are part of the API, whether they are visible to users or not, and 
they should follow the same convention as the rest of the API.

 From the framework perspective, subdomains are child objects of trusts, 
no different to how DNS records are child objects of DNS zones, except 
they are internal and not visible to users. You even implemented them 
this way in the patch, I just don't like how hackily it is done.

>
> The fact that you currently see these commands in 'ipa trust-*' family
> reflects the situation. -mod command is intended to be internal as well,
> it will not be visible in CLI in the end. The only command available to
> users will be 'ipa trust-domain-fetch'.

All the more reason for following the usual convention.

>
>> 2) There is already support for objects inside objects in the
>> framework, there's no need to reinvent this. See the parent_object
>> attribute of LDAPObject and the dns plugin for practical example.
> It does not work in this case properly. Feel free to try to implement
> it, really. I have spent good deal of last two weeks trying all options
> possible.

Why does it not work? The functionality of parent_object is pretty 
straightforward, so I can't imagine what might go wrong.

>>
>> 3) Create commands are usually named "*_add", not "*_create".
> The command is internal, see NO_CLI there. I'm not tied to particular
> naming but it is not visible to users.
>
>> 4) The "trust_domain_fetch" command gives the impression it operates
>> on top of a trust domain, but it actually operates on top of a trust.
>> I think it should be renamed to better reflect this.
> It operates on the trust. We don't have 'trust domains' at all. We have
> trusts and always had only trusts. Subdomains represent internal
> structure of the trust which is only visible to user for diagnostic
> purposes but practically users will not be able to modify them, only
> reference them for idranges.

Again, it doesn't matter whether it's internal or not, it should still 
follow the usual convention, for consistency, maintainability and code 
readability.

If trust_domain_fetch is the only one visible to users, I guess there's 
no harm in keeping the name.

Honza

-- 
Jan Cholasta