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

 -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
                       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.

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).

/ Alexander Bokovoy

Freeipa-devel mailing list

Reply via email to