[Freeipa-devel] [PATCHES 0001-0005] Profile management commands
Martin Basti
mbasti at redhat.com
Wed May 13 08:36:51 UTC 2015
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,
> 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
>
--
Martin Basti
More information about the Freeipa-devel
mailing list