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

[Martin Basti]

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