[Freeipa-devel] [PATCH] 728 default roles

[Martin Kosek]

On Tue, 2011-02-22 at 09:22 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Tue, 2011-02-22 at 13:14 +0100, Jan Zelený wrote:
> >> Rob Crittenden<rcritten at redhat.com>  wrote:
> >>> Jakub Hrozek wrote:
> >>>> On Mon, Feb 21, 2011 at 10:11:38AM -0500, Rob Crittenden wrote:
> >>>>> Rob Crittenden wrote:
> >>>>>> Jakub Hrozek wrote:
> >>>>>>> -----BEGIN PGP SIGNED MESSAGE-----
> >>>>>>> Hash: SHA1
> >>>>>>>
> >>>>>>> On 02/17/2011 04:35 AM, Rob Crittenden wrote:
> >>>>>>>> Add default roles and permissions for HBAC, SUDO and pw policy
> >>>>>>>>
> >>>>>>>> Created some default roles as examples. In doing so I realized that
> >>>>>>>> we were completely missing default rules for HBAC, SUDO and password
> >>>>>>>> policy so I added those as well.
> >>>>>>>>
> >>>>>>>> I ran into a problem when the updater has a default record and an add
> >>>>>>>> at the same time, it should handle it better now.
> >>>>>>>>
> >>>>>>>> ticket 585
> >>>>>>>>
> >>>>>>>> rob
> >>>>>>>
> >>>>>>> I'm not sure about the HBAC rules ACIs. They are specified as:
> >>>>>>>
> >>>>>>> 'target = "ldap:///cn=*,cn=hbac,$SUFFIX"'
> >>>>>>>
> >>>>>>> while HBAC rules' DN is:
> >>>>>>>
> >>>>>>> 'ipauniqueid=*,cn=hbac,$SUFFIX'.
> >>>>>>>
> >>>>>>> But HBAC rules do have a cn: attribute, so maybe the ACIs would work?
> >>>>>>
> >>>>>> No, you're right, this is wrong. I'll fix it up and resubmit.
> >>>>>>
> >>>>>>> The patch also needs rebasing on top of recent changes to
> >>>>>>> install/updates/Makefile.am
> >>>>>>>
> >>>>>>> Other than that, looks OK to me.
> >>>>>>>
> >>>>>>> btw when I was reviewing this patch, I noticed we add a "DNS
> >>>>>>> Administrators" privilege in dns.ldif. Would it make sense to add DNS
> >>>>>>> administration to "Security Architect" (replication management) and
> >>>>>>> "IT Specialist" (hosts management)?
> >>>>>>
> >>>>>> The DNS stuff is added only if DNS is enabled on the server so I can't
> >>>>>> add them by default.
> >>>>>>
> >>>>>> rob
> >>>>>
> >>>>> Updated patch.
> >>>>>
> >>>>> rob
> >>>>
> >>>> Interdiff looks fine, but I'm not able to apply the patch (not even
> >>>> 3-way merge), can you rebase?
> >>>
> >>> done
> >>
> >> The patch now applies ok (just one whitespace warning), ack
> >>
> >> Jan
> >>
> >> _______________________________________________
> >> Freeipa-devel mailing list
> >> Freeipa-devel at redhat.com
> >> https://www.redhat.com/mailman/listinfo/freeipa-devel
> >
> > I have to NACK this. I have found some issues in the new LDAP records:
> >
> > 1) A wrong groupdn for the following ACI in 40-delegation.update:
> > add:aci: '(target = "ldap:///cn=*,cn=sudorules,cn=sudo,$SUFFIX")(version
> > 3.0;acl "permission:Add SUDO rule";allow (add) groupdn = "ldap:///cn=Add
> > SUDOrule,cn=permissions,cn=pbac,$SUFFIX";)'
> >
> > It should be dap:///cn=Add SUDO rule,cn=permissions,cn=pbac,$SUFFIX
> >
> > 2) Another wrong target for few ACIs:
> > ldap:///cn=*,cn=sudorules,cn=sudo,$SUFFIX
> > is used instead of
> > ldap:///ipaUniqueID=*,cn=sudorules,cn=sudo,$SUFFIX
> >
> >
> > 3) Missing Description for the following new privileges:
> > Write IPA Configuration
> > Modify Users and Reset passwords
> > Modify Group membership
> >
> > Remainder looks good.
> >
> > Martin
> 
> Thanks for the careful review. Updated patch attached.
> 
> rob

Good job! Its OK now. ACK

Martin