Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
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.
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.
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
Can you use from ipalib import errors, instead?
6)
+def __exit__(self, exc_type, exc_value, traceback):
+Log out of the REST API
+# TODO logout
+self.cookie = None
This is forgotten, or will be this fixed later?
7)
+if not explanation: print resp_body # NOCOMMIT
These are all fixed for the next patchset.
Thanks!
Fraser
8) You can do:
Str('cn',
primary_key=True,
cli_name='id',
label=_('Profile ID'),
doc=_('Profile ID for referring to this profile'),
pattern='^[a-zA-Z]\w*$',
pattern_errmsg=_('invalid Profile ID'),
),
instead of:
profile_id_pattern = re.compile('^[a-zA-Z]\w*$')
def validate_profile_id(ugettext, value):
Ensure profile ID matches form required by CA.
if profile_id_pattern.match(value) is None:
return _('invalid Profile ID')
else:
return None
...
Str('cn', validate_profile_id,
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
Hi Jan, thanks for review. Comments inline.
On Wed, May 13, 2015 at 10:06:04AM +0200, 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.
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.
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
Can you use from ipalib import errors, instead?
6)
+def __exit__(self, exc_type, exc_value, traceback):
+Log out of the REST API
+# TODO logout
+self.cookie = None
This is forgotten, or will be this fixed later?
7)
+if not explanation: print resp_body # NOCOMMIT
These are all fixed for the next patchset.
Thanks!
Fraser
8) You can do:
Str('cn',
primary_key=True,
cli_name='id',
label=_('Profile ID'),
doc=_('Profile ID for referring to this profile'),
pattern='^[a-zA-Z]\w*$',
pattern_errmsg=_('invalid Profile ID'),
),
That is nice, I did not see this!
instead of:
profile_id_pattern = re.compile('^[a-zA-Z]\w*$')
def validate_profile_id(ugettext, value):
Ensure profile ID matches form required by CA.
if
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
Dne 13.5.2015 v 11:41 Fraser Tweedale napsal(a): Hi Jan, thanks for review. Comments inline. On Wed, May 13, 2015 at 10:06:04AM +0200, Jan Cholasta wrote: 12) IMO the profile backend should be merged in to the ra backend. I don't see a need to have these two separate. I wasn't sure, so I wrote them separately. I don't mind to make it part of ra, but because the code is working and having it separate makes it slightly easier to work on while developing these features, I will do this refactor later. OK. For now, can you at least rename it to something more descriptive, like ra_certprofile? -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
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.
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.
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
Can you use from ipalib import errors, instead?
6)
+def __exit__(self, exc_type, exc_value, traceback):
+Log out of the REST API
+# TODO logout
+self.cookie = None
This is forgotten, or will be this fixed later?
7)
+if not explanation: print resp_body # NOCOMMIT
These are all fixed for the next patchset.
Thanks!
Fraser
8) You can do:
Str('cn',
primary_key=True,
cli_name='id',
label=_('Profile ID'),
doc=_('Profile ID for referring to this profile'),
pattern='^[a-zA-Z]\w*$',
pattern_errmsg=_('invalid Profile ID'),
),
instead of:
profile_id_pattern = re.compile('^[a-zA-Z]\w*$')
def validate_profile_id(ugettext, value):
Ensure profile ID matches form required by CA.
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
On Wed, May 13, 2015 at 01:19:49PM +0200, Jan Cholasta wrote: Dne 13.5.2015 v 11:41 Fraser Tweedale napsal(a): Hi Jan, thanks for review. Comments inline. On Wed, May 13, 2015 at 10:06:04AM +0200, Jan Cholasta wrote: 12) IMO the profile backend should be merged in to the ra backend. I don't see a need to have these two separate. I wasn't sure, so I wrote them separately. I don't mind to make it part of ra, but because the code is working and having it separate makes it slightly easier to work on while developing these features, I will do this refactor later. OK. For now, can you at least rename it to something more descriptive, like ra_certprofile? No problem; updated name in next patchset. -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
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.
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.
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
Can you use from ipalib import errors, instead?
6)
+def __exit__(self, exc_type, exc_value, traceback):
+Log out of the REST API
+# TODO logout
+self.cookie = None
This is forgotten, or will be this fixed later?
7)
+if not explanation: print resp_body # NOCOMMIT
These are all fixed for the next patchset.
Thanks!
Fraser
8) You can do:
Str('cn',
primary_key=True,
cli_name='id',
label=_('Profile ID'),
doc=_('Profile ID for referring to this profile'),
pattern='^[a-zA-Z]\w*$',
pattern_errmsg=_('invalid Profile ID'),
),
instead of:
profile_id_pattern = re.compile('^[a-zA-Z]\w*$')
def validate_profile_id(ugettext, value):
Ensure profile ID matches form required by CA.
if profile_id_pattern.match(value) is None:
return _('invalid Profile ID')
else:
return None
...
Str('cn', validate_profile_id,
primary_key=True,
cli_name='id',
label=_('Profile ID'),
doc=_('Profile ID for
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
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. 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? 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? 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 Can you use from ipalib import errors, instead? 6) +def __exit__(self, exc_type, exc_value, traceback): +Log out of the REST API +# TODO logout +self.cookie = None This is forgotten, or will be this fixed later? 7) +if not explanation: print resp_body # NOCOMMIT These are all fixed for the next patchset. Thanks! Fraser Martin^2 -- Martin Basti -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
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 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 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 Can you use from ipalib import errors, instead? 6) +def __exit__(self, exc_type, exc_value, traceback): +Log out of the REST API +# TODO logout +self.cookie = None This is forgotten, or will be this fixed later? 7) +if not explanation: print resp_body # NOCOMMIT These are all fixed for the next patchset. Thanks! Fraser Martin^2 -- Martin Basti -- Martin Basti -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0001-0005] Profile management commands
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: 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 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 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. 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 Can you use from ipalib import errors, instead? 6) +def __exit__(self, exc_type, exc_value, traceback): +Log out of the REST API +# TODO logout +self.cookie = None This is forgotten, or will be this fixed later? 7) +if not explanation: print resp_body # NOCOMMIT Martin^2 -- Martin Basti -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
