Re: [Freeipa-devel] [PATCHES 0001-0007] Profile management
Dne 20.5.2015 v 07:56 Fraser Tweedale napsal(a): On Wed, May 20, 2015 at 07:40:44AM +0200, Jan Cholasta wrote: Dne 19.5.2015 v 13:50 Fraser Tweedale napsal(a): On Tue, May 19, 2015 at 10:52:49AM +0200, Jan Cholasta wrote: Dne 15.5.2015 v 14:27 Martin Basti napsal(a): On 15/05/15 10:24, Fraser Tweedale wrote: Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. To have everything CA-specific in one place and created only when CA is installed. This is consistent with DNS, the other optional IPA component. OK, I'll change it. Sub-CA data and Certificate Identity Mapping settings could also be stored under there, when implemented. Yes, Sub-CAs should also be stored there, but certificate identity mappings should work even without CA installed, so they should be stored somewhere else, like cn=etc. That makes sense. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance method called during install and in ipa-upgradeconfig. So more
Re: [Freeipa-devel] [PATCHES 0001-0007] Profile management
On Tue, May 19, 2015 at 10:52:49AM +0200, Jan Cholasta wrote: Dne 15.5.2015 v 14:27 Martin Basti napsal(a): On 15/05/15 10:24, Fraser Tweedale wrote: Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. To have everything CA-specific in one place and created only when CA is installed. This is consistent with DNS, the other optional IPA component. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance method called during install and in ipa-upgradeconfig. So more or less what Fraser already did, except the ipa-upgradeconfig part. Patch 0004 was updated and now has CAInstance method during install, and ipa-upgradeconfig method for upgrade. It would be better if you used the same CAInstance method both for install and upgrade, instead of duplicating the code. You shouldn't use the deprecated modify_s method of IPAdmin.
Re: [Freeipa-devel] [PATCHES 0001-0007] Profile management
Dne 15.5.2015 v 14:27 Martin Basti napsal(a): On 15/05/15 10:24, Fraser Tweedale wrote: Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. To have everything CA-specific in one place and created only when CA is installed. This is consistent with DNS, the other optional IPA component. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance method called during install and in ipa-upgradeconfig. So more or less what Fraser already did, except the ipa-upgradeconfig part. Patch 0004 was updated and now has CAInstance method during install, and ipa-upgradeconfig method for upgrade. It would be better if you used the same CAInstance method both for install and upgrade, instead of duplicating the code. You shouldn't use the deprecated modify_s method of IPAdmin. This is not very nice: +sysupgrade.set_upgrade_state(*upgrade_state_args + (True,)) Martin^2 Other issues: 1) I do not see modifications in API.txt file 2) We use new
Re: [Freeipa-devel] [PATCHES 0001-0007] Profile management
On Tue, May 19, 2015 at 10:52:49AM +0200, Jan Cholasta wrote: Dne 15.5.2015 v 14:27 Martin Basti napsal(a): On 15/05/15 10:24, Fraser Tweedale wrote: Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. To have everything CA-specific in one place and created only when CA is installed. This is consistent with DNS, the other optional IPA component. OK, I'll change it. Sub-CA data and Certificate Identity Mapping settings could also be stored under there, when implemented. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance method called during install and in ipa-upgradeconfig. So more or less what Fraser already did, except the ipa-upgradeconfig part. Patch 0004 was updated and now has CAInstance method during install, and ipa-upgradeconfig method for upgrade. It would be better if you used the same CAInstance method both
Re: [Freeipa-devel] [PATCHES 0001-0007] Profile management
On Wed, May 20, 2015 at 07:40:44AM +0200, Jan Cholasta wrote: Dne 19.5.2015 v 13:50 Fraser Tweedale napsal(a): On Tue, May 19, 2015 at 10:52:49AM +0200, Jan Cholasta wrote: Dne 15.5.2015 v 14:27 Martin Basti napsal(a): On 15/05/15 10:24, Fraser Tweedale wrote: Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. To have everything CA-specific in one place and created only when CA is installed. This is consistent with DNS, the other optional IPA component. OK, I'll change it. Sub-CA data and Certificate Identity Mapping settings could also be stored under there, when implemented. Yes, Sub-CAs should also be stored there, but certificate identity mappings should work even without CA installed, so they should be stored somewhere else, like cn=etc. That makes sense. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance
Re: [Freeipa-devel] [PATCHES 0001-0007] Profile management
Dne 19.5.2015 v 13:50 Fraser Tweedale napsal(a): On Tue, May 19, 2015 at 10:52:49AM +0200, Jan Cholasta wrote: Dne 15.5.2015 v 14:27 Martin Basti napsal(a): On 15/05/15 10:24, Fraser Tweedale wrote: Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. To have everything CA-specific in one place and created only when CA is installed. This is consistent with DNS, the other optional IPA component. OK, I'll change it. Sub-CA data and Certificate Identity Mapping settings could also be stored under there, when implemented. Yes, Sub-CAs should also be stored there, but certificate identity mappings should work even without CA installed, so they should be stored somewhere else, like cn=etc. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance method called during install and in ipa-upgradeconfig. So more or less what Fraser already did, except the ipa-upgradeconfig part. Patch 0004 was updated and now has CAInstance method during
[Freeipa-devel] [PATCHES 0001-0007] Profile management
Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance method called during install and in ipa-upgradeconfig. So more or less what Fraser already did, except the ipa-upgradeconfig part. Patch 0004 was updated and now has CAInstance method during install, and ipa-upgradeconfig method for upgrade. Martin^2 Other issues: 1) I do not see modifications in API.txt file 2) We use new shorter license header # # Copyright (C) 2015 FreeIPA Contributors see COPYING for license # 3) +from ipalib.plugins.baseldap import \ +LDAPObject, LDAPSearch, LDAPCreate, LDAPDelete, LDAPUpdate, LDAPRetrieve please use 'import ( modules, .. )' instead of '\' 4) +if method == 'POST' \ +and 'content-type' not in (str(k).lower() for k in headers.viewkeys()): again, please use
Re: [Freeipa-devel] [PATCHES 0001-0007] Profile management
On 15/05/15 10:24, Fraser Tweedale wrote: Please find attached latest patches including new patches: - 0006 enable LDAP-based profiles in Dogtag on upgrade - 0007 import included profiles during install or upgrade There is one TODO in the patches where some more code is needed on Dogtag side, and another TODO (not in patches) to migrate caIPAserviceCert profile to DefaultService profile and switch to using DefaultService for cerificate issuance (as the default profile). Jan and Martin, further comments to earlier reviews inline. Cheers, Fraser On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 10:36 Martin Basti napsal(a): On 13/05/15 10:06, Jan Cholasta wrote: Hi, Dne 5.5.2015 v 10:38 Martin Basti napsal(a): On 05/05/15 08:29, Fraser Tweedale wrote: On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote: On 04/05/15 15:36, Fraser Tweedale wrote: Hello, Please review the first cut of the 'certprofile' command and other changes associated with the Certificate Profiles feature[1]. Custom profiles can't be used yet because 'cert-request' has not been updated, but you can manage the profiles (find, show, import, modify, delete). There's a bit more work to do on profile management and a lot more to do for using profiles and sub-CAs. I am tracking my progress on etherpad[2] so if you are reviewing check there for the TODO list and some commentary. If you want to test: for f21, please use Dogtag from my copr[2]. For f22 the required version is in updates-testing (or my copr). In summary: this is not the whole feature, just the first functional part. Since it is my first experience developing in the IPA framework I want to get patches out so you can point out all the things I did wrong or overlooked, and I can fix them. Don't hold back :) [1] http://www.freeipa.org/page/V4/Certificate_Profiles [2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress [3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/ Thank you for patches, I have no idea what kind of dogtag magic is happening there, but I have a few comments related to IPA: Thanks for reviewing, Martin. Comments inline. You are welcome, comments inline. Martin^2 Upgrade: 1) +config.set(CA, pki_profiles_in_ldap, True) IMO this will work only for new installations. For upgrade you may need to add this to ipa-upgradeconfig OK. 2) +dn: cn=certprofiles,cn=etc,$SUFFIX +changetype: add +objectClass: nsContainer +objectClass: top +cn: certprofiles IMO this will work only for new installations. For upgrade you may need to add it into update file as well, with the 'default' keyword I don't understand about the 'default' keyword - can you expain this some more? In an upgrade file: dn: cn=certprofiles,cn=etc,$SUFFIX default:objectClass: nsContainer default:objectClass: top default:cn: certprofiles Maybe we should do what DNS does and have a container for CA specific stuff in the suffix: cn=ca,$SUFFIX. The container would be created only if CA is installed. Certificate profile container would then be cn=certprofiles,cn=ca,$SUFFIX. I haven't changed this for the current patchset. What are the implications / motivations for changing it. 3) Your patch 0004 will work on new installations only. You may need to add that new step into ipa-upgradeconfig. Must be that step there during installation? If not you can create just one update file, which will be applied at the end of installation and during upgrade. This change must be made to the Dogtag directory (not IPA) - can an update file be used to do that? If not, is ipa-upgradeconfig the best place to make this change? If it is change in LDAP, you can use updatefile: dn: cn=aclResources,$SUFFIX add:resourceACLS: certServer.profile.configuration:read,modify:allow (read,modify) group=Certificate Manager Agents:Certificate Manager agents may modify (create/update/delete) and read profiles Please temporarily use my patch freeipa-mbasti-231-4, (which will be pushed soon) to avoid issues with CSV Note that this update should be done only if CA is installed. In that case, you must create update plugins. I would prefer a CAInstance method called during install and in ipa-upgradeconfig. So more or less what Fraser already did, except the ipa-upgradeconfig part. Patch 0004 was updated and now has CAInstance method during install, and ipa-upgradeconfig method for upgrade. Martin^2 Other issues: 1) I do not see modifications in API.txt file 2) We use new shorter license header # # Copyright (C) 2015 FreeIPA Contributors see COPYING for license # 3) +from ipalib.plugins.baseldap import \ +LDAPObject, LDAPSearch, LDAPCreate, LDAPDelete, LDAPUpdate, LDAPRetrieve please use 'import ( modules, .. )' instead of '\' 4) +if method == 'POST' \ +and 'content-type' not in (str(k).lower() for k in headers.viewkeys()): again, please use if ( ... ): instead \ 5) +import ipalib.errors as errors in dogtag.py
