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

[Martin Kosek]

On 06/28/2012 03:20 PM, Petr Viktorin wrote:
> 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
> 

Pushed to master.

Martin