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

[Martin Kosek]

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