[Freeipa-devel] [PATCH] 0022 Use ipauniqueid for the RDN of sudo commands (rebased)

[Martin Kosek]

On Tue, 2012-03-13 at 13:43 +0100, Petr Viktorin wrote:
> On 03/12/2012 06:10 PM, Martin Kosek wrote:
> > On Mon, 2012-03-12 at 17:12 +0100, Petr Viktorin wrote:
> >> On 03/12/2012 04:01 PM, Martin Kosek wrote:
> >>> On Mon, 2012-03-12 at 14:38 +0100, Petr Viktorin wrote:
> >>>> On 03/12/2012 01:26 PM, Martin Kosek wrote:
> >>>>> On Thu, 2012-03-08 at 16:57 +0100, Petr Viktorin wrote:
> >>>>>> Since sudo commands are case-sensitive, we can't use the CN as the RDN.
> >>>>>> With this patch, the UUID is used instead.
> >>>>>> It seems like a too easy fix. What am I missing?
> >>>>>>
> >>>>>> As far as I understand, the fact that the DN has a different structure
> >>>>>> now shouldn't cause problems, even if there still are commands created
> >>>>>> by old IPA versions.
> >>>>>> For testing, use an unpatched version to create a few of these.
> >>>>>>
> >>>>>> The sudo commands are no longer sorted in sudocmd-find output. Doing
> >>>>>> that would require the ability to use an arbitrary attribute as sort
> >>>>>> key. Should I file an issue for that?
> >>>>>
> >>>>> I don't think that's necessary. We sort by LDAP object's primary key and
> >>>>> since new SUDO commands still have sudocmd as its primary key, the
> >>>>> sorting should just work (at least it does for me).
> >>>>
> >>>> Right, sorry for the noise.
> >>>>
> >>>>>>
> >>>>>> Tests for the case sensitivity are included.
> >>>>>>
> >>>>>> https://fedorahosted.org/freeipa/ticket/2482
> >>>>>
> >>>>> This works pretty fine. Both my old client tests and sudoers compat tree
> >>>>> tests looks good. So, cautious ACK from me.
> >>>>>
> >>>>> Martin
> >>>>>
> >>>>
> >>>> The attached version is rebased against my patch 20.
> >>>>
> >>>
> >>> Ah, I found an issue with the changed RDN attribute. We crash when I
> >>> delete sudocmd that sudorule has enrolled as a member:
> >>>
> >>> # ipa sudocmd-add bar1
> >>> # ipa sudocmd-add bar2
> >>> # ipa sudorule-add foo
> >>> # ipa sudorule-add-allow-command foo --sudocmds=bar1,bar2
> >>> # ipa sudocmd-del bar2
> >>> # ipa sudorule-find
> >>> ipa: ERROR: an internal error has occurred
> >>>
> >>> /var/log/httpd/error_log:
> >>> [Mon Mar 12 10:41:24 2012] [error] Traceback (most recent call last):
> >>> [Mon Mar 12 10:41:24 2012] [error]   File
> >>> "/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py",   line 315,
> >>> in wsgi_execute
> >>> [Mon Mar 12 10:41:24 2012] [error]     result =
> >>> self.Command[name](*args, **options)
> >>> [Mon Mar 12 10:41:24 2012] [error]   File
> >>> "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line  438, in
> >>> __call__
> >>> [Mon Mar 12 10:41:24 2012] [error]     ret = self.run(*args, **options)
> >>> [Mon Mar 12 10:41:24 2012] [error]   File
> >>> "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line  696, in run
> >>> [Mon Mar 12 10:41:24 2012] [error]     return self.execute(*args,
> >>> **options)
> >>> [Mon Mar 12 10:41:24 2012] [error]   File
> >>> "/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.   py", line
> >>> 1866, in execute
> >>> [Mon Mar 12 10:41:24 2012] [error]
> >>> self.obj.convert_attribute_members(e[1], *args, **options)
> >>> [Mon Mar 12 10:41:24 2012] [error]   File
> >>> "/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.   py", line
> >>> 518, in convert_attribute_members
> >>> [Mon Mar 12 10:41:24 2012] [error]
> >>> ldap_obj.get_primary_key_from_dn(member)
> >>> [Mon Mar 12 10:41:24 2012] [error]   File
> >>> "/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.   py", line
> >>> 490, in get_primary_key_from_dn
> >>> [Mon Mar 12 10:41:24 2012] [error]     return dn[self.primary_key.name]
> >>> [Mon Mar 12 10:41:24 2012] [error]   File
> >>> "/usr/lib/python2.7/site-packages/ipalib/dn.py", line 1137,  in
> >>> __getitem__
> >>> [Mon Mar 12 10:41:24 2012] [error]     raise KeyError("\\"%s\\" not
> >>> found in %s" % (key, self.         __str__()))
> >>>
> >>>
> >>> The problem is in this function:
> >>>       def get_primary_key_from_dn(self, dn):
> >>>           try:
> >>>               if self.rdn_attribute:
> >>>                   (dn, entry_attrs) = self.backend.get_entry(
> >>>                       dn, [self.primary_key.name]
> >>>                   )
> >>>                   try:
> >>>                       return entry_attrs[self.primary_key.name][0]
> >>>                   except (KeyError, IndexError):
> >>>                       return ''
> >>>           except errors.NotFound:
> >>>               pass
> >>>           # DN object assures we're returning a decoded (unescaped) value
> >>>           dn = DN(dn)
> >>>           return dn[self.primary_key.name]
> >>>
> >>> Martin
> >>>
> >>
> >> Should sudocmd-del should also delete the command from any rules the
> >> command is in?
> >
> > I would rather prevent deleting it when it is any sudorule to avoid
> > unpleasant user surprises.
> >
> >>
> >> That function needs to be fixed either way. But I'm not sure what it
> >> should do if the entry doesn't exist. Return the full DN instead?
> >>
> >
> > Probably, as this is the only value we know at the moment - it certainly
> > should not crash. Although DN with ipaUniqueID won't be very helpful. A
> > precaution I suggested above should prevent that.
> >
> > Martin
> >
> 
> Here is a partial patch to do that, I'll squash after addressing the ACI 
> problem. If you have time I'd welcome comments on this.
> 

Yup, the approach looks OK. I just think you don't have to create a new
error for this purpose but just use errors.DependentEntry that is
already used for the same purpose in hbacrule.py (selinuxusermaps are
checked if they don't point to the deleted hbacrule).

Martin