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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code