[Freeipa-devel] [PATCHES] 0521-0522 - Add managed read permissions to krbtpolicy & Allow anonymous read access to Kerberos realm container name

[Simo Sorce]

On Tue, 2014-04-15 at 16:48 +0200, Martin Kosek wrote:
> On 04/15/2014 03:16 PM, Simo Sorce wrote:
> > On Tue, 2014-04-15 at 13:13 +0200, Petr Viktorin wrote:
> >> On 04/15/2014 09:43 AM, Martin Kosek wrote:
> >>> On 04/15/2014 09:38 AM, Martin Kosek wrote:
> >>>> On 04/14/2014 07:18 PM, Simo Sorce wrote:
> >>>>> On Mon, 2014-04-14 at 18:54 +0200, Petr Viktorin wrote:
> >>>>>> Hello,
> >>>>>>
> >>>>>> The first patch adds default read permissions to krbtpolicy. Since the
> >>>>>> plugin manages entries in two trees, there are two permissions. Since
> >>>>>> two permissions are needed to cover krbtpolicy, it can't be used as a
> >>>>>> permission's --type.
> >>>>>> The permissions are added to a new privilege, 'Kerberos Ticket Policy
> >>>>>> Readers'.
> >>>>>>
> >>>>>> The second patch adds an ACI for reading the Kerberos realm name. Since
> >>>>>> client enrollment won't work without this, I don't see a reason for
> >>>>>> having it managed by a permission.
> >>>>>>
> >>>>>
> >>>>> LGTM
> >>>>>
> >>>>> Simo.
> >>>>>
> >>>>
> >>>> 521 breaks a unit test:
> >>>>
> >>>> ======================================================================
> >>>> FAIL: test_permission[37]: permission_find: Search for u'Testperm_RN' using
> >>>> --subtree
> >>>> ----------------------------------------------------------------------
> >>>> Traceback (most recent call last):
> >>>>    File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
> >>>>      self.test(*self.arg)
> >>>>    File "/root/freeipa-master/ipatests/test_xmlrpc/xmlrpc_test.py", line 301, in
> >>>> <lambda>
> >>>>      func = lambda: self.check(nice, **test)
> >>>>    File "/root/freeipa-master/ipatests/test_xmlrpc/xmlrpc_test.py", line 319, in
> >>>> check
> >>>>      self.check_output(nice, cmd, args, options, expected, extra_check)
> >>>>    File "/root/freeipa-master/ipatests/test_xmlrpc/xmlrpc_test.py", line 359, in
> >>>> check_output
> >>>>      assert_deepequal(expected, got, nice)
> >>>>    File "/root/freeipa-master/ipatests/util.py", line 344, in assert_deepequal
> >>>>      assert_deepequal(e_sub, g_sub, doc, stack + (key,))
> >>>>    File "/root/freeipa-master/ipatests/util.py", line 352, in assert_deepequal
> >>>>      VALUE % (doc, expected, got, stack)
> >>>> AssertionError: assert_deepequal: expected != got.
> >>>>    test_permission[37]: permission_find: Search for u'Testperm_RN' using --subtree
> >>>>    expected = 1
> >>>>    got = 2
> >>>>    path = ('count',)
> >>
> >> Thanks for the catch, tests updated.
> >>
> >>>> Otherwise it works fine (krbtpolicy-show for user cannot be tested yet as we
> >>>> miss permissions for users).
> >>
> >> Right; I don't think this permission by itself should allow access to 
> >> users. Correct me if that's wrong.
> >>
> >> I created a users permission for testing:
> >>     ipa permission-add 'allow reading user objectclass' --type user 
> >> --right={read,search,compare} --attrs objectclass --bind all
> >>
> >>> /me hit Send too soon.
> >>>
> >>> Although 522 works functionally and client now discovers the IPA server, there
> >>> is no path from SUFFIX to cn=REALM for anonymous users.
> >>>
> >>> I would personally change the ACI to
> >>>
> >>> (targetattr = "cn || objectclass")(targetfilter =
> >>> "(|(objectclass=krbrealmcontainer)(objectclass=krbcontainer))")(version 3.0;acl
> >>> "Anonymous read access to Kerberos container";allow (read,compare,search)
> >>> userdn = "ldap:///anyone";)'
> >>>
> >>> and put it to cn=kerberos,$SUFFIX (which is of krbcontainer objectclass).
> >>
> >> Right, that's necessary for UIs to list the container.
> >> Simo, are you okay with this?
> > 
> > It is no secret that an IPA server has a container named after the
> > domain. And the REALM name is available unauthenticated from DNS, so
> > knowledge of it's existence is given.
> > Therefore I see no problem if anonymous can see the container exists, as
> > long as no contents (beyond what we already determined need to be) are
> > revealed I see no problem.
> > 
> > Simo.
> > 
> > 
> 
> Patches work fine. The only problem I see that we now expose group password
> policies
> 
> # ldapsearch -h `hostname` -x -b
> 'cn=MKOSEK-FEDORA20.TEST,cn=kerberos,dc=mkosek-fedora20,dc=test' cn
> ...
> # MKOSEK-FEDORA20.TEST, kerberos, mkosek-fedora20.test
> dn: cn=MKOSEK-FEDORA20.TEST,cn=kerberos,dc=mkosek-fedora20,dc=test
> cn: MKOSEK-FEDORA20.TEST
> 
> # global_policy, MKOSEK-FEDORA20.TEST, kerberos, mkosek-fedora20.test
> dn: cn=global_policy,cn=MKOSEK-FEDORA20.TEST,cn=kerberos,dc=mkosek-fedora20,dc
>  =test
> cn: global_policy
> 
> # ipausers, MKOSEK-FEDORA20.TEST, kerberos, mkosek-fedora20.test
> dn: cn=ipausers,cn=MKOSEK-FEDORA20.TEST,cn=kerberos,dc=mkosek-fedora20,dc=test
> cn: ipausers
> ...
> 
> It seems suboptimal as we now moved access to group password policy to special
> permission and we probably do not want to leak a list of defined group policies.
> 
> Question is how to prevent it as the group password policies have nsContainer
> OC. I see 2 options:
> 
> 1) Update pwpolicy plugin to avoid using nsContainer for group password policy
> (I do not think it is needed, krbPwdPolicy contains cn attribute
> 
> 2) Update our container allowing ACI to not display it.
> 
> Option 1) would be nice, but I am afraid it would break search in older
> versions which expects the nsContainer OC to be there.
> 
> As for option 2) I am not sure how best to do it. It would be best to exclude
> both cn=etc and cn=kerberos subtrees, but I could not figure out an ACI syntax
> to do it.
> 
> Both
> (target != "ldap:///some=dn")(target != "ldap:///other=dn")
> and
> (target != "ldap:///some=dn" && target != "ldap:///other=dn")
> are not correct. CCing Ludwig to advise.
> 
> Alternative is to exclude the the password policies by targetfilter, but I
> think excluding whole tree is better.
> 
> Martin

It's like an LDAP filter but no quite so maybe:
(&(target != "ldap:///some=dn")(target != "ldap:///other=dn"))  ?

HTH,
Simo.