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

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

Reply via email to