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

Tue Mar 13 19:10:20 UTC 2012

On Tue, 2012-03-13 at 18:17 +0100, Petr Viktorin wrote:
> On 03/12/2012 04:44 PM, Martin Kosek wrote:
> > On Mon, 2012-03-12 at 16:01 +0100, 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
> >>
> >
> > I found one more issue, this one is more serious (I am glad my hunch was
> > not malfunctioning). This patch breaks ACIs for sudocmd, user with
> > appropriate privilege will no longer be able to add/modify/delete sudo
> > commands:
> >
> > # kinit fbar
> > Password for fbar at IDM.LAB.BOS.REDHAT.COM:
> > # ipa sudorule-add fbar
> > ----------------------
> > Added Sudo Rule "fbar"
> > ----------------------
> >    Rule name: fbar
> >    Enabled: TRUE
> > # ipa sudorule-del fbar
> > ------------------------
> > Deleted Sudo Rule "fbar"
> > ------------------------
> > # ipa sudocmd-add fbar1
> > ipa: ERROR: Insufficient access: Insufficient 'add' privilege to add the
> > entry
> > 'ipaUniqueID=c68b98ae-6c58-11e1-be75-00163e7228ea,cn=sudocmds,cn=sudo,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'.
> > # ipa sudocmd-del sudocmd1
> > ipa: ERROR: Insufficient access: Insufficient 'delete' privilege to
> > delete the entry
> > 'ipaUniqueID=0c7659d4-6c50-11e1-9b18-00163e7228ea,cn=sudocmds,cn=sudo,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'.
> > # ipa sudocmd-mod sudocmd1 --desc=foo
> > ipa: ERROR: Insufficient access: Insufficient 'write' privilege to the
> > 'description' attribute of entry
> > 'ipauniqueid=0c7659d4-6c50-11e1-9b18-00163e7228ea,cn=sudocmds,cn=sudo,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'
> >
> 
> Rob, I heard you're the ACI expert around here.
> 
> We can change the permission subtree to 
> ldap:///*=*,cn=sudocmds,cn=sudo,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com 
> which would work (*if* an upgrade goes smoothly). Or do a target filter, 
> but if I understand correctly, IPA permissions can't work with those so 
> we'd have to make sure those don't misbehave.
> Or drop the entire patch for the upcoming release, and leave Sudo 
> commands case insensitive. (There's potentially a security issue here. 
> Sudo commands are not just the names of the binaries, but also their 
> arguments.)
> 

I would not call it a security issue. Where do we violate the security?
AFAIU, the main issue here is that this bug basically prevents admin
from creating 2 sudocmds which differ just in a letter case. It does not
mean that our sudoers tree in LDAP or the sudo itself handles sudocmd
value in a case insensitive matter.

Martin