[Freeipa-devel] [PATCH 0030] Add option to specify SID using domain name to idrange-add/mod

[Martin Kosek]

On 02/18/2013 12:36 PM, Alexander Bokovoy wrote:
> On Fri, 15 Feb 2013, Tomas Babej wrote:
>> On 02/14/2013 05:37 PM, Alexander Bokovoy wrote:
>>> On Thu, 14 Feb 2013, Tomas Babej wrote:
>>>>>>>> + Str('ipanttrusteddomainname?',
>>>>>>>> + cli_name='dom_name',
>>>>>>>> + flags=('no_search', 'virtual_attribute'),
>>>>>>>> + label=_('Name of the trusted domain'),
>>>>>>>> + ),
>>>>>>> New options is added but API.txt wasn't changed. As result, 'make rpms'
>>>>>>> does not work.
>>>>>>>
>>>>>>> Could you please fix the patch and re-send it?
>>>>>>>
>>>>>> Sorry about that.
>>>>>>
>>>>>> Updated patch attached.
>>>>> I have one small question regarding use of dom_sid/dom_name.
>>>>>
>>>>> If both dom_sid and dom_name were specified, failing to resolve dom_name
>>>>> would force command to raise exception.
>>>>>
>>>>> I'm not sure this is right behavior. Probably we should detect that both
>>>>> dom_sid and dom_name were specified and bail out earlier so that only
>>>>> one of them is accepted. That would be clearer to users, wouldn't it
>>>> Sure, I definitely agree on that point. I added the check to idrange-add and
>>>> idrange-mod. Also, the patch needed a rebase to apply on the current master.
>>> I tried to play with various scenarious and one thing I noticed is that we
>>> refer dom_sid and dom_name in the error messages as they are
>>> named internally. CLI replaces underscore by hyphen (_ -> -) and therefore
>>> this error message becomes wrong -- you cannot specify --dom_sid, this
>>> option is unknown to CLI.
>>>
>>> In Web UI this would also mean an error message pointing to non-existing
>>> option. Perhaps it would be reasonable to name options '--name' and
>>> '--sid'? We don't have any clash there:
>>> -------------------------------------------------------------------------
>>> # ipa idrange-mod --help
>>> Usage: ipa [global-options] idrange-mod NAME [options]
>>>
>>> Positional arguments:
>>>  NAME                  Range name
>>>
>>> Options:
>>>  -h, --help            show this help message and exit
>>>  --base-id=INT         First Posix ID of the range
>>>  --range-size=INT      Number of IDs in the range
>>>  --rid-base=INT        First RID of the corresponding RID range
>>>  --secondary-rid-base=INT
>>>                        First RID of the secondary RID range
>>>  --dom-sid=STR         Domain SID of the trusted domain
>>>  --dom-name=STR        Name of the trusted domain
>>>  --setattr=STR         Set an attribute to a name/value pair. Format is
>>>                        attr=value. For multi-valued attributes, the command
>>>                        replaces the values already present.
>>>  --addattr=STR         Add an attribute/value pair. Format is
>>> attr=value. The
>>>                        attribute must be part of the schema.
>>>  --delattr=STR         Delete an attribute/value pair. The option willbe
>>>                        evaluated last, after all sets and adds.
>>>  --rights              Display the access rights of this entry(requires
>>>                        --all). See ipa man page for details.
>>>  --all                 Retrieve and print all attributes from the server.
>>>                        Affects command output.
>>>  --raw                 Print entries as stored on the server. Only affects
>>>                        output format.
>>> -------------------------------------------------------------------------
>>>
>>> So, if --name and --sid were used, an error message would become
>>> ----------------------------------------------------------------------
>>> # ipa idrange-mod AD.LAN_id_range --dom-name foo.bar ipa: ERROR: invalid 'ID
>>> Range setup': SID for the specified trusted
>>> domain name could not be found. Please specify the SID directly using
>>> --sid option.
>>> ----------------------------------------------------------------------
>>>
>>>
>>> Additionally, there is an error when SID for an object within the domain
>>> is specified. Last RID of the SID represents an object within the domain
>>> and we generally need to be careful allowing it in the place where
>>> domain SID is specified:
>>>
>>> # ipa idrange-mod AD.LAN_id_range --dom-sid
>>> S-1-5-21-3502988750-125904550-3683905862-1
>>> -----------------------------------
>>> Modified ID range "AD.LAN_id_range"
>>> -----------------------------------
>>>  Range name: AD.LAN_id_range
>>>  First Posix ID of the range: 1442800000
>>>  Number of IDs in the range: 200000
>>>  First RID of the corresponding RID range: 0
>>>  Domain SID of the trusted domain: S-1-5-21-3502988750-125904550-3683905862-1
>>>  Range type: Active Directory domain range
>>>
>>> Now this range is completely unusable due to the fact that there is no
>>> way to match the domain SID against the range.
>>>
>>> I think we need to make the check against established trusts more
>>> strict and only allow exact match.
>>>
>> 1.) Regarding dom_sid and dom_name options, we do not have to change their
>> internal names
>> to get more user-friendly error messages. These are hardcoded strings, and
>> not generated from
>> internal names of the options. I followed the naming convention already set
>> in the file, but you're right,
>> the current state is somewhat confusing to the end user. I changed all the
>> error messages so that
>> they refer to hyphen-versions of the options (not only dom_sid/dom_name but
>> also rid_base, etc.).
> Ok, thanks.
> 
>> I considered renaming the options to --sid and --name. However, although
>> --sid is pretty self-explanatory,
>> --name could be quite confusing, as ID range has name of its own.
>> Furthermore, this would break some
>> documentation / other references, and since refactoring of the error messages
>> described above solves our
>> issue here, I'd vote for that solution.
> Agreed.
> 
>> 2.) Exact match against estabilished trusts - this is a nice catch! However,
>> as far as I understand this is
>> not something introduced by my patch, and it would not be transparent to
>> include the fix here. If you agree,
>> I'll create a new ticket in Trac.
> Please do.
> 
> 
>> Updated patch attached (error messages refactored).
> ACK.
> 

Pushed to master, ipa-3-1 (with a mild rebase).

Martin