[Freeipa-devel] [PATCHES 0001-0005] Profile management commands

[Jan Cholasta]

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 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