[Freeipa-devel] [PATCH] Per-domain DNS update permissions

[Petr Viktorin]

On 06/28/2012 12:53 PM, Martin Kosek wrote:
> On 06/28/2012 11:20 AM, Petr Viktorin wrote:
>> On 06/27/2012 06:01 PM, Petr Viktorin wrote:
>>> On 06/27/2012 02:50 PM, Martin Kosek wrote:
>>>> On 06/25/2012 08:50 PM, Rob Crittenden wrote:
>>>>> Simo Sorce wrote:
>>>>>> On Fri, 2012-06-22 at 14:25 +0200, Martin Kosek wrote:
>>>>>>> On 06/22/2012 02:23 PM, Simo Sorce wrote:
>>>>>>>> On Fri, 2012-06-22 at 12:20 +0200, Martin Kosek wrote:
>>>>>>>>> On 06/18/2012 05:37 PM, Rob Crittenden wrote:
>>>>>>>>>> Martin Kosek wrote:
>>>>>>>>>>> On Fri, 2012-06-15 at 10:15 -0400, Simo Sorce wrote:
>>>>>>>>>>>> On Fri, 2012-06-15 at 15:22 +0200, Martin Kosek wrote:
>>>>>>>>>>>>> Hello all,
>>>>>>>>>>>>>
>>>>>>>>>>>>> In a scope of ticket 2511 I would like to implement an
>>>>>>>>>>>>> ability to
>>>>>>>>>>>>> delegate a DNS update permissions to chosen user (or host)
>>>>>>>>>>>>> without
>>>>>>>>>>>>> having to give the user full "Update DNS Entries" privileges,
>>>>>>>>>>>>> i.e.
>>>>>>>>>>>>> allow
>>>>>>>>>>>>> him to modify any DNS zone or record.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So far, this is what I would like to do (comments welcome):
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1) Create new objectclass "idnsManagedZone" with "managedBy"
>>>>>>>>>>>>> attribute
>>>>>>>>>>>>> in MAY list
>>>>>>>>>>>>> 2) Create new DNS commands:
>>>>>>>>>>>>>       a] dnszone-add-managedby [--users=USERS] [--hosts=HOSTS]
>>>>>>>>>>>>>       b] dnszone-remove-managedby [--users=USERS] [--hosts=HOSTS]
>>>>>>>>>>>>>       - these commands would add/remove chosen user/host DN to
>>>>>>>>>>>>> managedBy
>>>>>>>>>>>>> attribute in chosen DNS zone
>>>>>>>>>>>>> 3) Add new generic ACIs to cn=dns,$SUFFIX:
>>>>>>>>>>>>> aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version
>>>>>>>>>>>>> 3.0;acl
>>>>>>>>>>>>> "Users and hosts can add DNS entries";allow (add) userattr =
>>>>>>>>>>>>> "parent[1].managedby#USERDN";)
>>>>>>>>>>>>> ... add similar ACIs for UPDATE, REMOVE access
>>>>>>>>>>>>>
>>>>>>>>>>>>> With these steps done, all that an administrator would need
>>>>>>>>>>>>> to do to
>>>>>>>>>>>>> delegate a management of a DNS zone "example.com" is to run this
>>>>>>>>>>>>> command:
>>>>>>>>>>>>> $ ipa dnszone-add-managedby example.com --users=fbar
>>>>>>>>>>>>>
>>>>>>>>>>>>> The only downside I found so far is that the user would
>>>>>>>>>>>>> already need to
>>>>>>>>>>>>> have "Read DNS Entries" permission assigned, otherwise he
>>>>>>>>>>>>> would not be
>>>>>>>>>>>>> able to actually read DNS entries (allow rules can't take
>>>>>>>>>>>>> precedence
>>>>>>>>>>>>> over deny rule we implemented to deny public access to DNS
>>>>>>>>>>>>> tree).
>>>>>>>>>>>>>
>>>>>>>>>>>>> An admin could of course create a special privilege and role
>>>>>>>>>>>>> with just
>>>>>>>>>>>>> "Read DNS Entries" permission and then assign it to relevant
>>>>>>>>>>>>> users/groups, but this looks awkward. Any idea to make this
>>>>>>>>>>>>> simpler?
>>>>>>>>>>>>> Maybe creating a group "dns readers" by default which would
>>>>>>>>>>>>> allow such
>>>>>>>>>>>>> access?
>>>>>>>>>>>>
>>>>>>>>>>>> Change the deny rule to deny to everyone except the user in
>>>>>>>>>>>> "parent[1].managedby#USERDN" ?
>>>>>>>>>>>>
>>>>>>>>>>>> Simo.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Good idea, I will do that. I will just use
>>>>>>>>>>> "parent[0,1].managedby#USERDN" so that user can also read the zone
>>>>>>>>>>> record. This way, a selected user will have read/write access
>>>>>>>>>>> to the
>>>>>>>>>>> chosen zone only, which is exactly what we want to achieve.
>>>>>>>>>>
>>>>>>>>>> Yes, this sounds workable to me too.
>>>>>>>>>>
>>>>>>>>>> rob
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There were some second thoughts about the proposed design, which
>>>>>>>>> I would
>>>>>>>>> like to discuss so that we can eventually accept another (better)
>>>>>>>>> solution for this feature.
>>>>>>>>>
>>>>>>>>> The main concern here was that proposed solution (based on user
>>>>>>>>> list in
>>>>>>>>> managedBy attribute in DNS zone) is not in line with the rest of
>>>>>>>>> permission&privilege architecture in IPA.
>>>>>>>>>
>>>>>>>>> Here is another idea how to address the feature (I tested it and it
>>>>>>>>> would work):
>>>>>>>>> 1) Get rid of the deny rule on cn=dns,$SUFFIX by modifying global
>>>>>>>>> access
>>>>>>>>> rule (a working patch attached) to avoid current and future
>>>>>>>>> issues with
>>>>>>>>> extending ACIs (deny rules are evil).
>>>>>>>>>
>>>>>>>>> 2) Add new Managed Entry Definition and Template to automatically
>>>>>>>>> add
>>>>>>>>> "Manage DNS zone $idsname" permission. These could be used with
>>>>>>>>> standard
>>>>>>>>> IPA privileges, roles and thus could be assigned to users, groups,
>>>>>>>>> hosts, hostgroups...
>>>>>>>>>
>>>>>>>>> 3) New DNS zone managedBy attribute won't be manageable by user,
>>>>>>>>> but it
>>>>>>>>> will hold a DN of the managed Permission entry
>>>>>>>>>
>>>>>>>>> 4) Add the following ACIs to cn=dns,$SUFFIX:
>>>>>>>>> aci: (targetattr = "*")
>>>>>>>>> (version 3.0; acl "Read DNS entries"; allow (read,search,compare)
>>>>>>>>> userattr = "parent[0,1].managedby#GROUPDN";)
>>>>>>>>>
>>>>>>>>> aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")
>>>>>>>>> (version 3.0;acl "Add dns entries";allow (add)
>>>>>>>>> userattr = "parent[1].managedby#GROUPDN";)
>>>>>>>>>
>>>>>>>>> aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")
>>>>>>>>> (version 3.0;acl "Remove DNS entries";allow (delete)
>>>>>>>>> userattr = "parent[1].managedby#GROUPDN";)
>>>>>>>>>
>>>>>>>>> aci: (targetattr = "idnsname || cn || idnsallowdynupdate ||
>>>>>>>>> dnsttl ||
>>>>>>>>> dnsclass || arecord || aaaarecord || a6record || nsrecord ||
>>>>>>>>> cnamerecord
>>>>>>>>> || ptrrecord || srvrecord || txtrecord || mxrecord || mdrecord   ||
>>>>>>>>> hinforecord || minforecord || afsdbrecord || sigrecord ||
>>>>>>>>> keyrecord ||
>>>>>>>>> locrecord || nxtrecord ||     naptrrecord || kxrecord ||
>>>>>>>>> certrecord ||
>>>>>>>>> dnamerecord || dsrecord || sshfprecord || rrsigrecord ||
>>>>>>>>> nsecrecord ||
>>>>>>>>> idnsname || idnszoneactive || idnssoamname || idnssoarname ||
>>>>>>>>> idnssoaserial || idnssoarefresh || idnssoaretry || idnssoaexpire ||
>>>>>>>>> idnssoaminimum || idnsupdatepolicy || idnsallowquery ||
>>>>>>>>> idnsallowtransfer || idnsallowsyncptr || idnsforwardpolicy ||
>>>>>>>>> idnsforwarders")
>>>>>>>>> (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl
>>>>>>>>> "Update
>>>>>>>>> DNS Entries";allow (write) userattr =
>>>>>>>>> "parent[0,1].managedby#GROUPDN";)
>>>>>>>>>
>>>>>>>>> I needed to add permission DN to the managedBy attribute so that
>>>>>>>>> I could
>>>>>>>>> create just one set of generic ACIs without having to create a
>>>>>>>>> set of
>>>>>>>>> ACIs for every new zone and thus let users with "Update DNS entries"
>>>>>>>>> permission have a write access to the "aci" attribute.
>>>>>>>>>
>>>>>>>>> Would this design be better than the previous one? Comments welcome.
>>>>>>>>
>>>>>>>> Removing Deny ACIs would be great.
>>>>>>>> But don't we need a second set of ACIs to allow uber admins to still
>>>>>>>> control all zones ? or is that part of current ACIs not going to
>>>>>>>> change ?
>>>>>>>>
>>>>>>>> Simo.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks to the removal of the deny rule, this would be already
>>>>>>> allowed by
>>>>>>> this existing ACI:
>>>>>>>
>>>>>>> aci: (targetattr != "userPassword || krbPrincipalKey ||
>>>>>>> sambaLMPassword
>>>>>>> || sambaNTPassword || passwordHistory || krbMKey ||
>>>>>>> krbPrincipalName ||
>>>>>>> krbCanonicalName || krbUPEnabled || krbTicketPolicyReference ||
>>>>>>> krbPrincipalExpiration || krbPasswordExpiration ||
>>>>>>> krbPwdPolicyReference
>>>>>>> || krbPrincipalType || krbPwdHistory || krbLastPwdChange ||
>>>>>>> krbPrincipalAliases || krbExtraData || krbLastSuccessfulAuth ||
>>>>>>> krbLastFailedAuth || krbLoginFailedCount || krbTicketFlags ||
>>>>>>> ipaUniqueId || memberOf || serverHostName || enrolledBy")(version 3.0;
>>>>>>> acl "Admin can manage any entry"; allow (all) groupdn =
>>>>>>> "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)
>>>>>>
>>>>>> Oh right!
>>>>>> I like it even more then :-)
>>>>>>
>>>>>> Simo.
>>>>>>
>>>>>
>>>>> Yes, this looks like it will work and eliminating a deny rule is a
>>>>> definite plus.
>>>>>
>>>>> rob
>>>>
>>>> I have finished a patch based on the second design. IMO it is indeed
>>>> better -
>>>> no deny ACI for DNS and just a standard permission for per-zone access
>>>> delegation.
>>>>
>>>> There is just one difference from the proposed design draft: per-zone
>>>> permissions are not created automatically by Managed Entry plugin, but
>>>> rather
>>>> manually and only for DNS zones where per-zone access is needed. There
>>>> is a new
>>>> command for that - dnszone-add-permission.
>>>>
>>>> This will leave permission tree cleaner + we won't have to deal with all
>>>> Managed Entry plugin machinery.
>>>>
>>>> More details can be found in a commit message.
>>>>
>>>> Martin
>>>>
>>>
>>> In permission_add_noaci.get_options, it would be better to filter out
>>> the `permission.aci_attributes`. If you only allow ('all', 'raw',
>>> 'permissiontype'), the list will have to be updated whenever a new
>>> global option is added.
>>> (This would happen for ticket #2732; I'll want to make the 'version'
>>> argument explicit for all commands.)
>
> Right, I fixed the option generation to rather simply filter out ACI attribute
> list we already have available.
>
>>>
>>>
>>> Unprivileged users can find out if a zone is defined by trying to delete
>>> it. Is this expected behavior?
>>>
>>> $ ipa dnszone-del idm.lab.bos.redhat.com
>>> ipa: ERROR: Insufficient access: Insufficient 'delete' privilege to
>>> delete the entry
>>> 'idnsname=idm.lab.bos.redhat.com,cn=dns,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'.
>>>
>>> $ ipa dnszone-del does.not.exist
>>> ipa: ERROR: does.not.exist: DNS zone not found
>
> Yeah, that's true. Not sure if we can do much about it, that's how LDAP
> behaves. But since no actual record value is returned to the user he could only
> try to brute-force the LDAP structure. This should not hurt, he could as well
> try to brute force the DNS records via DNS queries which would even give him
> more information.
>
> Bottom line is that I think that current ACIs are right, but I can be convinced
> with a better solution...
>
>>>
>>>
>>> The patch works well on upgrade. Tomorrow I'll test a fresh install.
>>>
>>
>> One more comment: there is no error message when removing a permission that
>> doesn't exist:
>>
>> $ ./ipa dnszone_remove_permission idm.lab.bos.redhat.com
>> ------------------------------------------------------------------
>> Removed system permission "Manage DNS zone idm.lab.bos.redhat.com"
>> ----------------------------------------------------------------
>> $ ./ipa dnszone_remove_permission idm.lab.bos.redhat.com
>> ------------------------------------------------------------------
>> Removed system permission "Manage DNS zone idm.lab.bos.redhat.com"
>> ----------------------------------------------------------------
>>
>> I found no other issues.
>>
>
> Fixed:
> # ipa dnszone-remove-permission example.com
> -------------------------------------------------------
> Removed system permission "Manage DNS zone example.com"
> -------------------------------------------------------
> # ipa dnszone-remove-permission example.com
> ipa: ERROR: Manage DNS zone example.com: permission not found
>
> Martin
>

ACK

-- 
Petr³