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

Wed May 13 09:41:53 UTC 2015

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 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).
> 
OK.

> 
> 10) All the commands should call ipalib.plugins.cert.ca_enabled_check().
> 
OK.

> 
> 11) I think the File parameter of certprofile_import should be called just
> 'file'.
> 
OK.

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

Thanks,
Fraser

> 
> Honza
> 
> -- 
> Jan Cholasta