[Freeipa-devel] [PATCHES 0001-0005] Profile management commands
Jan Cholasta
jcholast at redhat.com
Wed May 13 08:39:55 UTC 2015
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."""
>> 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 referring to this profile'),
>> ),
>>
>> 9) Please don't invent new attributes (ipaCertProfileSummary) when you
>> can use existing ones (description).
>>
>>
>> 10) All the commands should call ipalib.plugins.cert.ca_enabled_check().
>>
>>
>> 11) I think the File parameter of certprofile_import should be called
>> just 'file'.
>>
>>
>> 12) IMO the profile backend should be merged in to the ra backend. I
>> don't see a need to have these two separate.
>>
>>
>> Honza
>>
>
>
--
Jan Cholasta
More information about the Freeipa-devel
mailing list