Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
Simo Sorce wrote: On Mon, 2014-06-16 at 09:53 +0200, Petr Viktorin wrote: On 06/13/2014 10:20 PM, Simo Sorce wrote: [...] 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? No, nothing in the short term. Ok, then I think attached is the patch 0003 we want. This changes admins superpowers to not allow ipaProtectedOperation by default and instead adds a specific right in cn=accounts so admin can keep fetching keytabs for any principal. We may want to turn this into a permission with a future patch. Upgrade in F-20 fails: Upgrade failed with ACL Syntax Error(-5):(targetattr=\22ipaProtectedOperation;write_keys\22)(version 3.0; acl \22Admins are allowed to rekey any entity\22; allow(write) groupdn = \22ldap:///cn=admins: Invalid syntax. IPA upgrade failed. You also have $SUFFIX hardcoded as dc=ipa,dc=dev,dc=lan here and in default-aci.ldif . I think the fix is to quote the whole thing like: -add:aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,dc=ipa,dc=dev,dc=lan;;) +add:aci: '(targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;)' I don't know if this is your issue or not, but after fixing that the upgrade still fails with: Upgrade failed with unknown object class ipaVirtualOperation: 2014-06-17T19:17:20Z DEBUG Final value after applying updates 2014-06-17T19:17:20Z DEBUG dn: cn=request certificate,cn=virtual operations,cn=etc,dc=greyoak,dc=com 2014-06-17T19:17:20Z DEBUG objectClass: 2014-06-17T19:17:20Z DEBUG nsContainer 2014-06-17T19:17:20Z DEBUG top 2014-06-17T19:17:20Z DEBUG ipaVirtualOperation 2014-06-17T19:17:20Z DEBUG cn: 2014-06-17T19:17:20Z DEBUG request certificate 2014-06-17T19:17:20Z DEBUG [(0, u'objectClass', ['ipaVirtualOperation'])] 2014-06-17T19:17:20Z DEBUG Live 1, updated 1 2014-06-17T19:17:20Z ERROR Upgrade failed with unknown object class ipaVirtualOperation On update the global admin ACI is not changed to add ipaProtectedOperation to the list of protected attributes. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-17 at 15:30 -0400, Rob Crittenden wrote: Simo Sorce wrote: On Mon, 2014-06-16 at 09:53 +0200, Petr Viktorin wrote: On 06/13/2014 10:20 PM, Simo Sorce wrote: [...] 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? No, nothing in the short term. Ok, then I think attached is the patch 0003 we want. This changes admins superpowers to not allow ipaProtectedOperation by default and instead adds a specific right in cn=accounts so admin can keep fetching keytabs for any principal. We may want to turn this into a permission with a future patch. Upgrade in F-20 fails: Upgrade failed with ACL Syntax Error(-5):(targetattr=\22ipaProtectedOperation;write_keys\22)(version 3.0; acl \22Admins are allowed to rekey any entity\22; allow(write) groupdn = \22ldap:///cn=admins: Invalid syntax. IPA upgrade failed. You also have $SUFFIX hardcoded as dc=ipa,dc=dev,dc=lan here and in default-aci.ldif . I think the fix is to quote the whole thing like: Arghh :( Let me fix that, sorry. -add:aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,dc=ipa,dc=dev,dc=lan;;) +add:aci: '(targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;)' I don't know if this is your issue or not, but after fixing that the upgrade still fails with: Upgrade failed with unknown object class ipaVirtualOperation: I know nothing about this. I do not touch or manipulate this in any way. 2014-06-17T19:17:20Z DEBUG Final value after applying updates 2014-06-17T19:17:20Z DEBUG dn: cn=request certificate,cn=virtual operations,cn=etc,dc=greyoak,dc=com 2014-06-17T19:17:20Z DEBUG objectClass: 2014-06-17T19:17:20Z DEBUG nsContainer 2014-06-17T19:17:20Z DEBUG top 2014-06-17T19:17:20Z DEBUG ipaVirtualOperation 2014-06-17T19:17:20Z DEBUG cn: 2014-06-17T19:17:20Z DEBUG request certificate 2014-06-17T19:17:20Z DEBUG [(0, u'objectClass', ['ipaVirtualOperation'])] 2014-06-17T19:17:20Z DEBUG Live 1, updated 1 2014-06-17T19:17:20Z ERROR Upgrade failed with unknown object class ipaVirtualOperation On update the global admin ACI is not changed to add ipaProtectedOperation to the list of protected attributes. uhmm can you show me exactly what your current ACI looks like ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
Simo Sorce wrote: On Tue, 2014-06-17 at 15:30 -0400, Rob Crittenden wrote: Simo Sorce wrote: On Mon, 2014-06-16 at 09:53 +0200, Petr Viktorin wrote: On 06/13/2014 10:20 PM, Simo Sorce wrote: [...] 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? No, nothing in the short term. Ok, then I think attached is the patch 0003 we want. This changes admins superpowers to not allow ipaProtectedOperation by default and instead adds a specific right in cn=accounts so admin can keep fetching keytabs for any principal. We may want to turn this into a permission with a future patch. Upgrade in F-20 fails: Upgrade failed with ACL Syntax Error(-5):(targetattr=\22ipaProtectedOperation;write_keys\22)(version 3.0; acl \22Admins are allowed to rekey any entity\22; allow(write) groupdn = \22ldap:///cn=admins: Invalid syntax. IPA upgrade failed. You also have $SUFFIX hardcoded as dc=ipa,dc=dev,dc=lan here and in default-aci.ldif . I think the fix is to quote the whole thing like: Arghh :( Let me fix that, sorry. -add:aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,dc=ipa,dc=dev,dc=lan;;) +add:aci: '(targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;)' I don't know if this is your issue or not, but after fixing that the upgrade still fails with: Upgrade failed with unknown object class ipaVirtualOperation: I know nothing about this. I do not touch or manipulate this in any way. Ok, I guess we'll need someone to take a look at that, maybe it's a generic 4.0 upgrade issue. 2014-06-17T19:17:20Z DEBUG Final value after applying updates 2014-06-17T19:17:20Z DEBUG dn: cn=request certificate,cn=virtual operations,cn=etc,dc=greyoak,dc=com 2014-06-17T19:17:20Z DEBUG objectClass: 2014-06-17T19:17:20Z DEBUG nsContainer 2014-06-17T19:17:20Z DEBUG top 2014-06-17T19:17:20Z DEBUG ipaVirtualOperation 2014-06-17T19:17:20Z DEBUG cn: 2014-06-17T19:17:20Z DEBUG request certificate 2014-06-17T19:17:20Z DEBUG [(0, u'objectClass', ['ipaVirtualOperation'])] 2014-06-17T19:17:20Z DEBUG Live 1, updated 1 2014-06-17T19:17:20Z ERROR Upgrade failed with unknown object class ipaVirtualOperation On update the global admin ACI is not changed to add ipaProtectedOperation to the list of protected attributes. uhmm can you show me exactly what your current ACI looks like ? Sorry false alarm, I misread the aci output. Functionally the patch otherwise looks ok. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-17 at 15:49 -0400, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2014-06-17 at 15:30 -0400, Rob Crittenden wrote: Simo Sorce wrote: On Mon, 2014-06-16 at 09:53 +0200, Petr Viktorin wrote: On 06/13/2014 10:20 PM, Simo Sorce wrote: [...] 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? No, nothing in the short term. Ok, then I think attached is the patch 0003 we want. This changes admins superpowers to not allow ipaProtectedOperation by default and instead adds a specific right in cn=accounts so admin can keep fetching keytabs for any principal. We may want to turn this into a permission with a future patch. Upgrade in F-20 fails: Upgrade failed with ACL Syntax Error(-5):(targetattr=\22ipaProtectedOperation;write_keys\22)(version 3.0; acl \22Admins are allowed to rekey any entity\22; allow(write) groupdn = \22ldap:///cn=admins: Invalid syntax. IPA upgrade failed. You also have $SUFFIX hardcoded as dc=ipa,dc=dev,dc=lan here and in default-aci.ldif . I think the fix is to quote the whole thing like: Arghh :( Let me fix that, sorry. -add:aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,dc=ipa,dc=dev,dc=lan;;) +add:aci: '(targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;)' I don't know if this is your issue or not, but after fixing that the upgrade still fails with: Upgrade failed with unknown object class ipaVirtualOperation: I know nothing about this. I do not touch or manipulate this in any way. Ok, I guess we'll need someone to take a look at that, maybe it's a generic 4.0 upgrade issue. 2014-06-17T19:17:20Z DEBUG Final value after applying updates 2014-06-17T19:17:20Z DEBUG dn: cn=request certificate,cn=virtual operations,cn=etc,dc=greyoak,dc=com 2014-06-17T19:17:20Z DEBUG objectClass: 2014-06-17T19:17:20Z DEBUG nsContainer 2014-06-17T19:17:20Z DEBUG top 2014-06-17T19:17:20Z DEBUG ipaVirtualOperation 2014-06-17T19:17:20Z DEBUG cn: 2014-06-17T19:17:20Z DEBUG request certificate 2014-06-17T19:17:20Z DEBUG [(0, u'objectClass', ['ipaVirtualOperation'])] 2014-06-17T19:17:20Z DEBUG Live 1, updated 1 2014-06-17T19:17:20Z ERROR Upgrade failed with unknown object class ipaVirtualOperation On update the global admin ACI is not changed to add ipaProtectedOperation to the list of protected attributes. uhmm can you show me exactly what your current ACI looks like ? Sorry false alarm, I misread the aci output. Functionally the patch otherwise looks ok. rob Ok attached corrected patch to remove the embarrassing wrong suffix. Simo. -- Simo Sorce * Red Hat, Inc * New York From e71bea3d2ec31224049a042ca93a15be2f1e6038 Mon Sep 17 00:00:00 2001 From: Simo Sorce s...@redhat.com Date: Tue, 17 Sep 2013 00:30:14 -0400 Subject: [PATCH 3/6] keytab: Add new extended operation to get a keytab. This new extended operation allow to create new keys or retrieve existing ones. The new set of keys is returned as a ASN.1 structure similar to the one that is passed in by the 'set keytab' extended operation. Access to the operation is regulated through a new special ACI that allows 'retrieval' only if the user has access to an attribute named ipaProtectedOperation postfixed by the subtypes 'read_keys' and 'write_keys' to distinguish between creation and retrieval operation. For example for allowing retrieval by a specific user the following ACI is set on cn=accounts: (targetattr=ipaProtectedOperation;read_keys) ... ... userattr=ipaAllowedToPerform;read_keys#USERDN) This ACI matches only if the service object hosts a new attribute named ipaAllowedToPerform that holds the DN of the user attempting the operation. Resolves: https://fedorahosted.org/freeipa/ticket/3859 --- .../ipa-pwd-extop/ipa_pwd_extop.c | 571 + install/share/60basev3.ldif| 3 + install/share/default-aci.ldif | 7 + install/updates/20-aci.update | 13 +- util/ipa_krb5.h| 1 + 5 files changed, 594 insertions(+), 1 deletion(-) diff --git
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On 06/13/2014 10:20 PM, Simo Sorce wrote: [...] 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? No, nothing in the short term. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Fri, 2014-06-13 at 23:16 +0200, Tomas Babej wrote: --- a/install/share/default-aci.ldif +++ b/install/share/default-aci.ldif @@ -21,11 +21,17 @@ changetype: modify add: aci aci: (targetfilter = (|(objectClass=ipaConfigObject)(dnahostname=*)))(version 3.0;acl Admins can change GUI config; allow (delete) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;) -dn: cn=accounts,$SUFFIX +dn: cngaccounts,$SUFFIX Ouch, thanks a lot! Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
Simo Sorce wrote: On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote: 0001 When is_allowed_to_access_attr() fails it should include the value of access in the error log for debugging. Ok added more detailed logging Nit: Coluld not fetch REALM backend Fixed There are still a ton of ber_scanf failed duplicated fatal errors. I'm fine keeping a common err_msg but the fatal error should be unique. Yeah thanks to this comment, I had a small change of heart. Instead of sending such detailed information to clients I reverted to send a little less information to the clients and instead LOG_FATAL in a more detailed way. HTH This breaks normal host delegation. If you add a host to another host's managedby, getting the keytab will fail. This is due to: [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny write on entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys) to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com: no aci matched the subject by aci(97): aciname= Groups allowed to create keytab keys, acidn=cn=accounts,dc=example,dc=com Ok this should be working now, I added a new ACI to allow also managedby#USERDN to operate on keytabs. New patches attached. Functionally these seem to work ok. I think there should be some documented way to enable the -r in ipa-getkeytab. Right now I'm not even entirely sure how one would add a permission to do so. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Fri, 2014-06-13 at 12:54 -0400, Rob Crittenden wrote: Simo Sorce wrote: On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote: 0001 When is_allowed_to_access_attr() fails it should include the value of access in the error log for debugging. Ok added more detailed logging Nit: Coluld not fetch REALM backend Fixed There are still a ton of ber_scanf failed duplicated fatal errors. I'm fine keeping a common err_msg but the fatal error should be unique. Yeah thanks to this comment, I had a small change of heart. Instead of sending such detailed information to clients I reverted to send a little less information to the clients and instead LOG_FATAL in a more detailed way. HTH This breaks normal host delegation. If you add a host to another host's managedby, getting the keytab will fail. This is due to: [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny write on entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys) to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com: no aci matched the subject by aci(97): aciname= Groups allowed to create keytab keys, acidn=cn=accounts,dc=example,dc=com Ok this should be working now, I added a new ACI to allow also managedby#USERDN to operate on keytabs. New patches attached. Functionally these seem to work ok. I think there should be some documented way to enable the -r in ipa-getkeytab. Right now I'm not even entirely sure how one would add a permission to do so. rob ATM the only way is to add the ipaAllowedOperations objectclass to the object you want to allow retrieving a keyt from and the ipaAllowedToPerform;reasd_key attribute Example: dn: test/foo.example@example.com,cn=services,cn=accounts,dc=example,dc=com changetype: modify add: objectclass objectclass: ipaAllowedOperations - add: ipaAllowedToPerform;read_key ipaAllowedToPerform;reasd_key: uid=cluster-admin,cn=users,cn=accounts,dc=example,dc=com Once you do this the user called cluster-admin will be allowed to retrieve the keytab w/o changing it. Of course you can list there a group or another host/service DN. HTH, Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Fri, 2014-06-13 at 14:04 -0400, Simo Sorce wrote: On Fri, 2014-06-13 at 12:54 -0400, Rob Crittenden wrote: Simo Sorce wrote: On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote: 0001 When is_allowed_to_access_attr() fails it should include the value of access in the error log for debugging. Ok added more detailed logging Nit: Coluld not fetch REALM backend Fixed There are still a ton of ber_scanf failed duplicated fatal errors. I'm fine keeping a common err_msg but the fatal error should be unique. Yeah thanks to this comment, I had a small change of heart. Instead of sending such detailed information to clients I reverted to send a little less information to the clients and instead LOG_FATAL in a more detailed way. HTH This breaks normal host delegation. If you add a host to another host's managedby, getting the keytab will fail. This is due to: [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny write on entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys) to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com: no aci matched the subject by aci(97): aciname= Groups allowed to create keytab keys, acidn=cn=accounts,dc=example,dc=com Ok this should be working now, I added a new ACI to allow also managedby#USERDN to operate on keytabs. New patches attached. Functionally these seem to work ok. I think there should be some documented way to enable the -r in ipa-getkeytab. Right now I'm not even entirely sure how one would add a permission to do so. rob ATM the only way is to add the ipaAllowedOperations objectclass to the object you want to allow retrieving a keyt from and the ipaAllowedToPerform;reasd_key attribute Example: dn: test/foo.example@example.com,cn=services,cn=accounts,dc=example,dc=com changetype: modify add: objectclass objectclass: ipaAllowedOperations - add: ipaAllowedToPerform;read_key ipaAllowedToPerform;reasd_key: uid=cluster-admin,cn=users,cn=accounts,dc=example,dc=com Once you do this the user called cluster-admin will be allowed to retrieve the keytab w/o changing it. Of course you can list there a group or another host/service DN. Doh, I realized we haven't created a feature page for this, I am going to create one now, so that the UI work we'll need in future can look it up and information like the above is registered. Will be available here: http://www.freeipa.org/page/V4/Keytab_Retrieval Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
Rob Crittenden wrote: Simo Sorce wrote: On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote: 0001 When is_allowed_to_access_attr() fails it should include the value of access in the error log for debugging. Ok added more detailed logging Nit: Coluld not fetch REALM backend Fixed There are still a ton of ber_scanf failed duplicated fatal errors. I'm fine keeping a common err_msg but the fatal error should be unique. Yeah thanks to this comment, I had a small change of heart. Instead of sending such detailed information to clients I reverted to send a little less information to the clients and instead LOG_FATAL in a more detailed way. HTH This breaks normal host delegation. If you add a host to another host's managedby, getting the keytab will fail. This is due to: [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny write on entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys) to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com: no aci matched the subject by aci(97): aciname= Groups allowed to create keytab keys, acidn=cn=accounts,dc=example,dc=com Ok this should be working now, I added a new ACI to allow also managedby#USERDN to operate on keytabs. New patches attached. Functionally these seem to work ok. I think there should be some documented way to enable the -r in ipa-getkeytab. Right now I'm not even entirely sure how one would add a permission to do so. NACK Simo noticed that the ACIs are in cn=accounts. On the one hand this is a reasonable place to put these, on the other it is a bit broad. I think we'll need type-specific ACIs in a number of subtrees: users, computers and services. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Fri, 2014-06-13 at 14:29 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Simo Sorce wrote: On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote: 0001 When is_allowed_to_access_attr() fails it should include the value of access in the error log for debugging. Ok added more detailed logging Nit: Coluld not fetch REALM backend Fixed There are still a ton of ber_scanf failed duplicated fatal errors. I'm fine keeping a common err_msg but the fatal error should be unique. Yeah thanks to this comment, I had a small change of heart. Instead of sending such detailed information to clients I reverted to send a little less information to the clients and instead LOG_FATAL in a more detailed way. HTH This breaks normal host delegation. If you add a host to another host's managedby, getting the keytab will fail. This is due to: [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny write on entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys) to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com: no aci matched the subject by aci(97): aciname= Groups allowed to create keytab keys, acidn=cn=accounts,dc=example,dc=com Ok this should be working now, I added a new ACI to allow also managedby#USERDN to operate on keytabs. New patches attached. Functionally these seem to work ok. I think there should be some documented way to enable the -r in ipa-getkeytab. Right now I'm not even entirely sure how one would add a permission to do so. NACK Simo noticed that the ACIs are in cn=accounts. On the one hand this is a reasonable place to put these, on the other it is a bit broad. I think we'll need type-specific ACIs in a number of subtrees: users, computers and services. [Only patch 3 attached, as none of the others changed, and addiotional discussion is needed, see below.] Ok after looking carefully into this problem I see that there are really 2 issues. 1) managedby for users, we do not want someone adding a managedby attribute to inadvertently allow the manager to set a user's password. So I changed that ACI and restricted it only to ipaHost and ipaService objects (tested). I haven't touched any other ACI because in order to use them you need to have intentionally added an ipaAllowedToPerform attribute to the user entry. 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? Otherwise we can add ipaProtectedOperation in the exclude list for the superACI (Admins can manage any entry) and add the following ACI in cn=accounts, to restore admin ability to set keys (but not retrieve them): aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn=ldap :///cn=admins,cn=groups,cn=accounts,$SUFFIX;) I tested this combination and it effectively stops admin from retrieving all keys unless explicitly authorize by positive ACIs/ipaAllowedToPerform attributes. Thoughts ? -- Simo Sorce * Red Hat, Inc * New York From e3a19bf910f2e2bbaabb870471045597469e790e Mon Sep 17 00:00:00 2001 From: Simo Sorce s...@redhat.com Date: Tue, 17 Sep 2013 00:30:14 -0400 Subject: [PATCH 3/6] keytab: Add new extended operation to get a keytab. This new extended operation allow to create new keys or retrieve existing ones. The new set of keys is returned as a ASN.1 structure similar to the one that is passed in by the 'set keytab' extended operation. Access to the operation is regulated through a new special ACI that allows 'retrieval' only if the user has access to an attribute named ipaProtectedOperation postfixed by the subtypes 'read_keys' and 'write_keys' to distinguish between creation and retrieval operation. For example for allowing retrieval by a specific user the following ACI is set on cn=accounts: (targetattr=ipaProtectedOperation;read_keys) ... ... userattr=ipaAllowedToPerform;read_keys#USERDN) This ACI matches only if the service object hosts a new attribute named ipaAllowedToPerform that holds the DN of the user attempting the operation. Resolves: https://fedorahosted.org/freeipa/ticket/3859 --- .../ipa-pwd-extop/ipa_pwd_extop.c | 571 + install/share/60basev3.ldif| 3 + install/share/default-aci.ldif | 8 +- install/updates/20-aci.update
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On 06/13/2014 10:20 PM, Simo Sorce wrote: On Fri, 2014-06-13 at 14:29 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Simo Sorce wrote: On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote: 0001 When is_allowed_to_access_attr() fails it should include the value of access in the error log for debugging. Ok added more detailed logging Nit: Coluld not fetch REALM backend Fixed There are still a ton of ber_scanf failed duplicated fatal errors. I'm fine keeping a common err_msg but the fatal error should be unique. Yeah thanks to this comment, I had a small change of heart. Instead of sending such detailed information to clients I reverted to send a little less information to the clients and instead LOG_FATAL in a more detailed way. HTH This breaks normal host delegation. If you add a host to another host's managedby, getting the keytab will fail. This is due to: [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny write on entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys) to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com: no aci matched the subject by aci(97): aciname= Groups allowed to create keytab keys, acidn=cn=accounts,dc=example,dc=com Ok this should be working now, I added a new ACI to allow also managedby#USERDN to operate on keytabs. New patches attached. Functionally these seem to work ok. I think there should be some documented way to enable the -r in ipa-getkeytab. Right now I'm not even entirely sure how one would add a permission to do so. NACK Simo noticed that the ACIs are in cn=accounts. On the one hand this is a reasonable place to put these, on the other it is a bit broad. I think we'll need type-specific ACIs in a number of subtrees: users, computers and services. [Only patch 3 attached, as none of the others changed, and addiotional discussion is needed, see below.] Ok after looking carefully into this problem I see that there are really 2 issues. 1) managedby for users, we do not want someone adding a managedby attribute to inadvertently allow the manager to set a user's password. So I changed that ACI and restricted it only to ipaHost and ipaService objects (tested). I haven't touched any other ACI because in order to use them you need to have intentionally added an ipaAllowedToPerform attribute to the user entry. 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? Otherwise we can add ipaProtectedOperation in the exclude list for the superACI (Admins can manage any entry) and add the following ACI in cn=accounts, to restore admin ability to set keys (but not retrieve them): aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl Admins are allowed to rekey any entity; allow(write) groupdn=ldap :///cn=admins,cn=groups,cn=accounts,$SUFFIX;) I tested this combination and it effectively stops admin from retrieving all keys unless explicitly authorize by positive ACIs/ipaAllowedToPerform attributes. Thoughts ? Just a heads up, I managed to catch a typo with my sleepy eyes: --- a/install/share/default-aci.ldif +++ b/install/share/default-aci.ldif @@ -21,11 +21,17 @@ changetype: modify add: aci aci: (targetfilter = (|(objectClass=ipaConfigObject)(dnahostname=*)))(version 3.0;acl Admins can change GUI config; allow (delete) groupdn = ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;) -dn: cn=accounts,$SUFFIX +dn: cngaccounts,$SUFFIX ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-10 at 20:13 -0400, Simo Sorce wrote: Still upgrading my server, so still untested, but again just to catch style issues, I'll post news once I can test the changes do not break functionality. I finished upgrading the server and redone my functional testing. Both getting ad setting keys seem to work as expected with the last batch of patches. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
Simo Sorce wrote: On Tue, 2014-06-10 at 14:27 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. Yes although it looks a bit ugly a long time ago we decided that we'd move to space indenting for big blocks of code, or keep tab indent only for minor modification. In the hope of eventually converting the remaining tabs. While we are here I decided to split setkeytab along the lines of getkeytab too, HTH. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Uhhmm now that we check an arbitrary attribute I need to change it. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. The generic 389ds function is slapi_access_allowed() which is normally sufficient, is_allowed_to_access_attr() is just a wrapper around some additional boilerplate that is not normally needed. Anyway feel free to open a bug in 389ds's trac. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. Well, kind of arbitrary, but ok 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. ok I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm not talking about ipapwd_setkeytab(). I'm talking about ipapwd_getkeytab(). This is entirely new code. There are very clear spots where it could be broken up. I consider this the biggest issue in this
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2014-06-10 at 14:27 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. Yes although it looks a bit ugly a long time ago we decided that we'd move to space indenting for big blocks of code, or keep tab indent only for minor modification. In the hope of eventually converting the remaining tabs. While we are here I decided to split setkeytab along the lines of getkeytab too, HTH. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Uhhmm now that we check an arbitrary attribute I need to change it. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. The generic 389ds function is slapi_access_allowed() which is normally sufficient, is_allowed_to_access_attr() is just a wrapper around some additional boilerplate that is not normally needed. Anyway feel free to open a bug in 389ds's trac. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. Well, kind of arbitrary, but ok 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. ok I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm not talking about ipapwd_setkeytab(). I'm talking about ipapwd_getkeytab(). This is entirely new code. There are very clear spots where it could be broken up. I consider this the biggest issue in this set of patches for two important reasons: 1. It makes the function really hard to review. 2. It is one of the most security/policy sensitive part of the code. These two are a bad combo. Much better! I was a bit disappointed that decode_getkeytab_request()/encode_getkeytab_reply() don't output/input a single request/response struct, but
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-10 at 14:27 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm not talking about ipapwd_setkeytab(). I'm talking about ipapwd_getkeytab(). This is entirely new code. There are very clear spots where it could be broken up. I consider this the biggest issue in this set of patches for two important reasons: 1. It makes the function really hard to review.
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-10 at 16:24 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 14:27 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm not talking about ipapwd_setkeytab(). I'm talking about ipapwd_getkeytab(). This is entirely new
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 Needs a rebase. I did my best for testing. In check_service_name() why not just initialize name to NULL rather than assigning it twice? Or is the value of name undefined if krb5_* fails? get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() I think the Access Strategy comment needs to be expanded (or dropped) in is_allowed_to_access_keys() In the fatal error in is_allowed_to_access_keys() should the error include the bound user and the resource they attempted to modify? 0002 ACK 0003 Nit: typo in commit, extedned I'd prefer unique error messages for each ber decode failure. Why is write access always required in ipapwd_getkeytab()? Would there be a case where a principal can only re-fetch existing keys? Since getnew is defined as a boolean in the ASN.1, is the conditional if (getnew == 0) preferred over just if (getnew)? Some errors read Out of Memory and others allocation failed. I think with a fatal error the line number is included in the error. If I'm wrong then perhaps something specific to the allocation should be included. The error message to the call ber_decode_krb5_key_data() doesn't seem to match. 0004 More duplicated error messages, e.g. Missing reply control 0005 More duplicated error messages, Failed to parse key data Typo, Incopatible Functionality-wise. Retrieving a keytab from a service without one fails in a strange way: # ipa-getkeytab -r -s `hostname` -p test1/`hostname` -k /tmp/test.kt Operation failed! Insufficient access rights Failed to get keytab I suppose a new permission is needed to add the ability to re-fetch keytabs. Should the admins group be able to do this out-of-the box? Maybe this is ok, I don't know, but it looks wierd. I fetched with -r the same keytab a number of times and this is what it contains: # klist -kt /tmp/test.kt Keytab name: FILE:/tmp/test.kt KVNO Timestamp Principal --- -- 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:51 test1/grindle.example@example.com 1 06/09/2014 13:36:51 test1/grindle.example@example.com 1 06/09/2014 13:36:51 test1/grindle.example@example.com The krbPrincipalKey value remained constant. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote:
On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote:
Simo Sorce wrote:
This patch set is an initial implementation of ticket #3859
It seem to be working fine in my initial testing but I have not yet
tested all cases.
However I wonted to throw it on the list to get some initial feedback
about the choices I made wrt access control and ipa-getkeytab flags and
default behavior.
In particular, the current patch set would require us to make
host/service keytabs readable by the requesting party (whoever that is,
admin or host itself) in order to allow it to get back the actual
keytab. I am not sure this is ideal. Also write access to the keytab is
still all is needed to allow someone to change it.
Neither is ideal, but it was simpler as a first implementation. In
particular I think we should allow either permission indipendently, and
it should be something an admin can change. However I do not like
allowing normal writes or reads to these attributes, mostly because w/o
access to the master key nobody can really make sense of actually
reading out the contents of KrbPrincipalKey or could write a blob that
can be successfully decrypted.
So I was wondering if we might want to prevent both reading and writing
via LDAP (except via extended operations) and instead use another method
to determine access patterns.
As for ipa-getkeytab is everyone ok with tryin the new method first and
always falling back to the old one (if a password has been provided) ?
0001
get_realm_backend() should probably have a comment on why returning NULL
is ok (either because there is no sdn or because there is no be). It
appears that things will eventually fail in get_entry_by_principal()
Instead of adding complex explanations I immediately return with an
error if get_realm_backend() returns NULL.
The logic here is correct, it just reads awkwardly. It is probably fine
as is.
There appear to be indiscriminate tab indents throughout the patch.
Please changes these to spaces.
I'm a bit confused by the memory management in get_realm_backend() and
its callers. Who owns 'be'?
0002
ACK
One small nitpick, then I too say ACK. In the commit message, the second
sentence doesn't need a line break.
0003
Same as patch 002: unnecessary line breaks in the commit message.
I think ipapwd_getkeytab() is unnecessarily long. Several sections of it
could be broken out into functions and would make it much more readable.
Nearly forty lines of variable declarations is a bit much. :) You could
break apart BER encoding/decoding, key writing, etc.
The ipapwd_getkeytab() function variable declarations contain a mix of
camel case and underscore styles.
The comment containing the ASN.1 code contains invalid syntax.
I find code like this very hard to read:
if (rtag == (ctag | 2)) {
Some named constants would be helpful here, or maybe a descriptively
named macro function.
We have C99 now, so I'd prefer local scope iterators in for loops:
for (int i = 0; ...) -- rather than -- for (i = 0; ...)
This has inconsistent indents:
+svals = ipapwd_encrypt_encode_key(krbcfg, data,
+ kenctypes ? num_kenctypes :
+krbcfg-num_pref_encsalts,
+ kenctypes ? kenctypes :
+krbcfg-pref_encsalts,
+ errMesg);
Has the new OID been registered?
Since getnew is defined as a boolean in the ASN.1, is the conditional
if (getnew == 0)
preferred over just
if (getnew)?
Future proof if we want to change it to a non-boolean value for whatever
reason in the future ? :)
I agree with rcrit. Fix this. :)
0004
More occasional indentation issues (tab vs space).
Local loop iterators preferred (for example: get_control_data()).
I'm not a fan of the output variable name for ipa_ldap_bind().
Other than that, pretty close to ACK on this one.
0005
Unnecessary line breaks in git commit message.
ASN.1 syntax errors. Also, this comment has some rogue tab indents.
In ldap_get_keytab(), can't the big while loop be a for loop with a
local scope iterator? Same with the for loop in
ipa_string_to_enctypes().
Line 308 (int retrieve = 0;) has an 8 space indent. This was likely to
match the tab indents of the surrounding code.
0006
ACK
Nathaniel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote:
On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote:
On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote:
Simo Sorce wrote:
This patch set is an initial implementation of ticket #3859
It seem to be working fine in my initial testing but I have not yet
tested all cases.
However I wonted to throw it on the list to get some initial feedback
about the choices I made wrt access control and ipa-getkeytab flags and
default behavior.
In particular, the current patch set would require us to make
host/service keytabs readable by the requesting party (whoever that is,
admin or host itself) in order to allow it to get back the actual
keytab. I am not sure this is ideal. Also write access to the keytab is
still all is needed to allow someone to change it.
Neither is ideal, but it was simpler as a first implementation. In
particular I think we should allow either permission indipendently, and
it should be something an admin can change. However I do not like
allowing normal writes or reads to these attributes, mostly because w/o
access to the master key nobody can really make sense of actually
reading out the contents of KrbPrincipalKey or could write a blob that
can be successfully decrypted.
So I was wondering if we might want to prevent both reading and writing
via LDAP (except via extended operations) and instead use another method
to determine access patterns.
As for ipa-getkeytab is everyone ok with tryin the new method first and
always falling back to the old one (if a password has been provided) ?
0001
get_realm_backend() should probably have a comment on why returning NULL
is ok (either because there is no sdn or because there is no be). It
appears that things will eventually fail in get_entry_by_principal()
Instead of adding complex explanations I immediately return with an
error if get_realm_backend() returns NULL.
The logic here is correct, it just reads awkwardly. It is probably fine
as is.
There appear to be indiscriminate tab indents throughout the patch.
Please changes these to spaces.
There are because the coed is old, I do not change the style of a piece
of code if we are just changing a few lines.
You need to read the patch in the context of the code to seen it.
I'm a bit confused by the memory management in get_realm_backend() and
its callers. Who owns 'be'?
The main DS code afaik.
0002
ACK
One small nitpick, then I too say ACK. In the commit message, the second
sentence doesn't need a line break.
I try to keep comments within 72 chars (though sometimes I forget and go
past till 80), which is why there is a line break there.
0003
Same as patch 002: unnecessary line breaks in the commit message.
See above.
I think ipapwd_getkeytab() is unnecessarily long. Several sections of it
could be broken out into functions and would make it much more readable.
That has already been done :-)
You should see the original ipa_setkeytab() ...
Nearly forty lines of variable declarations is a bit much. :) You could
break apart BER encoding/decoding, key writing, etc.
Perhaps, but wouldn't change the amount of code, unless you say it is
necessary in order to do a better review I will skip doing that for now.
The ipapwd_getkeytab() function variable declarations contain a mix of
camel case and underscore styles.
Inherited from old code, see ipa_setkeytab()
The comment containing the ASN.1 code contains invalid syntax.
Please be more specific ?
That pseudo code is not meant to be compiled, so it is a bit liberal.
I find code like this very hard to read:
if (rtag == (ctag | 2)) {
Some named constants would be helpful here, or maybe a descriptively
named macro function.
We have C99 now, so I'd prefer local scope iterators in for loops:
for (int i = 0; ...) -- rather than -- for (i = 0; ...)
We still declare everything upfront in freeipa code, not going to change
style with these patches.
This has inconsistent indents:
+svals = ipapwd_encrypt_encode_key(krbcfg, data,
+ kenctypes ? num_kenctypes :
+krbcfg-num_pref_encsalts,
+ kenctypes ? kenctypes :
+krbcfg-pref_encsalts,
+ errMesg);
Yes these indents are not the best but allow to keep the code within 80
chars.
Has the new OID been registered?
Yup.
Since getnew is defined as a boolean in the ASN.1, is the conditional
if (getnew == 0)
preferred over just
if (getnew)?
Future proof if we want to change it to a non-boolean value for whatever
reason in the future ? :)
I agree with rcrit. Fix this. :)
Fixing how ? There is nothing
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote:
On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote:
On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote:
On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote:
Simo Sorce wrote:
This patch set is an initial implementation of ticket #3859
It seem to be working fine in my initial testing but I have not yet
tested all cases.
However I wonted to throw it on the list to get some initial feedback
about the choices I made wrt access control and ipa-getkeytab flags
and
default behavior.
In particular, the current patch set would require us to make
host/service keytabs readable by the requesting party (whoever that
is,
admin or host itself) in order to allow it to get back the actual
keytab. I am not sure this is ideal. Also write access to the keytab
is
still all is needed to allow someone to change it.
Neither is ideal, but it was simpler as a first implementation. In
particular I think we should allow either permission indipendently,
and
it should be something an admin can change. However I do not like
allowing normal writes or reads to these attributes, mostly because
w/o
access to the master key nobody can really make sense of actually
reading out the contents of KrbPrincipalKey or could write a blob that
can be successfully decrypted.
So I was wondering if we might want to prevent both reading and
writing
via LDAP (except via extended operations) and instead use another
method
to determine access patterns.
As for ipa-getkeytab is everyone ok with tryin the new method first
and
always falling back to the old one (if a password has been provided) ?
0001
get_realm_backend() should probably have a comment on why returning NULL
is ok (either because there is no sdn or because there is no be). It
appears that things will eventually fail in get_entry_by_principal()
Instead of adding complex explanations I immediately return with an
error if get_realm_backend() returns NULL.
The logic here is correct, it just reads awkwardly. It is probably fine
as is.
There appear to be indiscriminate tab indents throughout the patch.
Please changes these to spaces.
There are because the coed is old, I do not change the style of a piece
of code if we are just changing a few lines.
You need to read the patch in the context of the code to seen it.
If it were just the problem you alluded to, I wouldn't have called it
out. I'm referring to tabs in the middle of new code that uses spaces.
This is most likely the result of copy/paste. Either write all the new
code to use tabs or match the copy/pasted lines to the surrounding new
code (my preference).
0002
ACK
One small nitpick, then I too say ACK. In the commit message, the second
sentence doesn't need a line break.
I try to keep comments within 72 chars (though sometimes I forget and go
past till 80), which is why there is a line break there.
Yeah, it just looks bad when sent over email, which is why I noticed it.
0003
Same as patch 002: unnecessary line breaks in the commit message.
See above.
I think ipapwd_getkeytab() is unnecessarily long. Several sections of it
could be broken out into functions and would make it much more readable.
That has already been done :-)
You should see the original ipa_setkeytab() ...
I'm not talking about ipapwd_setkeytab(). I'm talking about
ipapwd_getkeytab(). This is entirely new code. There are very clear
spots where it could be broken up. I consider this the biggest issue in
this set of patches for two important reasons:
1. It makes the function really hard to review.
2. It is one of the most security/policy sensitive part of the code.
These two are a bad combo.
Nearly forty lines of variable declarations is a bit much. :) You could
break apart BER encoding/decoding, key writing, etc.
Perhaps, but wouldn't change the amount of code, unless you say it is
necessary in order to do a better review I will skip doing that for now.
See above.
The ipapwd_getkeytab() function variable declarations contain a mix of
camel case and underscore styles.
Inherited from old code, see ipa_setkeytab()
Sure, but it is also a brand new section. Doesn't your editor have a
variable renamer? Mine does. ;)
The comment containing the ASN.1 code contains invalid syntax.
Please be more specific ?
That pseudo code is not meant to be compiled, so it is a bit liberal.
::= is, I think, the only issue.
I find code like this very hard to read:
if (rtag == (ctag | 2)) {
Some named constants would be helpful here, or maybe a descriptively
named macro function.
We have C99 now, so I'd prefer local scope iterators in for loops:
for (int i = 0; ...) -- rather than -- for (i =
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Thu, 2014-05-29 at 18:57 +0200, Petr Spacek wrote: On 29.5.2014 18:40, Nathaniel McCallum wrote: On Mon, 2013-09-23 at 08:12 -0400, Simo Sorce wrote: On Mon, 2013-09-23 at 09:00 +0200, Petr Spacek wrote: On 20.9.2013 21:35, Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? Simo. I was always curious why we don't use normal kadmin protocol? :-) Kadmin won't respect LDAP based ACIs, it always operate as root against LDAP, This leads to question: Why? Wouldn't be approach with S4U sufficient? and has it's own simple ACL file. We want to be able to easily manage and delegate access to keytab creation. We might try to change the kadmin code to impersonate the user but I think it would be invasive, I never seriously looked into it though. IMHO we have two options: A) Operate as root use LDAP proxy control. -- Determine DN of an object representing principal used by user. -- Use LDAP proxy control with given DN. B) Use S4U and connect to LDAP with user's credentials. Wouldn't such an approach have inherently better security properties (and potentially gain other operations for free)? The current patch set is also invasive (at least in terms of its size). If the kadmin approach is a similar sized or smaller change, I'd probably prefer that. I agree with Nathaniel. We have discussed this with MIT already, it is an important change to kadmin and will not be available even in 1.13, so that is not an option for now. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On 20.9.2013 21:35, Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? Simo. I was always curious why we don't use normal kadmin protocol? :-) -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Mon, 2013-09-23 at 09:00 +0200, Petr Spacek wrote: On 20.9.2013 21:35, Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? Simo. I was always curious why we don't use normal kadmin protocol? :-) Kadmin won't respect LDAP based ACIs, it always operate as root against LDAP, and has it's own simple ACL file. We want to be able to easily manage and delegate access to keytab creation. We might try to change the kadmin code to impersonate the user but I think it would be invasive, I never seriously looked into it though. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
