On Tue, May 19, 2015 at 10:52:49AM +0200, Jan Cholasta wrote:
> Dne 15.5.2015 v 14:27 Martin Basti napsal(a):
> >On 15/05/15 10:24, Fraser Tweedale wrote:
> >>Please find attached latest patches including new patches:
> >>
> >>- 0006 enable LDAP-based profiles in Dogtag on upgrade
> >>- 0007 import included profiles during install or upgrade
> >>
> >>There is one TODO in the patches where some more code is needed on
> >>Dogtag side, and another TODO (not in patches) to migrate
> >>caIPAserviceCert profile to DefaultService profile and switch to
> >>using DefaultService for cerificate issuance (as the default
> >>profile).
> >>
> >>Jan and Martin, further comments to earlier reviews inline.
> >>
> >>Cheers,
> >>Fraser
> >>
> >>On Wed, May 13, 2015 at 10:39:55AM +0200, Jan Cholasta wrote:
> >>>Dne 13.5.2015 v 10:36 Martin Basti napsal(a):
> >>>>On 13/05/15 10:06, 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.
> >>>>>
> >>I haven't changed this for the current patchset.  What are the
> >>implications / motivations for changing it.
> 
> To have everything CA-specific in one place and created only when CA is
> installed. This is consistent with DNS, the other optional IPA component.
> 
OK, I'll change it.  Sub-CA data and Certificate Identity Mapping
settings could also be stored under there, when implemented.

> >>
> >>>>>>>>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.
> >>>>In that case, you must create update plugins.
> >>>I would prefer a CAInstance method called during install and in
> >>>ipa-upgradeconfig. So more or less what Fraser already did, except the
> >>>ipa-upgradeconfig part.
> >>>
> >>Patch 0004 was updated and now has CAInstance method during install,
> >>and ipa-upgradeconfig method for upgrade.
> 
> It would be better if you used the same CAInstance method both for install
> and upgrade, instead of duplicating the code.
> 
I will unify the logic in next patch set.

> You shouldn't use the deprecated modify_s method of IPAdmin.
> 
> This is not very nice:
> 
> +    sysupgrade.set_upgrade_state(*upgrade_state_args + (True,))
> 
Ok, I will repeat the arguments for get_upgrade_state and
set_upgrade_state.

> >>
> >>>>Martin^2
> >>>>>>>>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'),
> >>>>>        ),
> >>>>>
> >>This is nice, but I have kept the separate method so that the
> >>cert-request command can use the same routine for validating the
> >>profile id (this will be in a subsequent patch).
> >>
> >>>>>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'.
> >>>>>
> >>9, 10, 11 were addressed for this patchset.
> >>
> >>>>>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
> >Thank you.
> >
> >I did part of review, again I have not idea about the dogtag magic
> >there, so I might be completely wrong.
> >Martin^2
> >
> >Patches need rebase.
> >
> >Patch 0001:
> >1)
> >I'm not sure if this is added during upgrade
> >+        config.set("CA", "pki_profiles_in_ldap", "True")
> >
> >Patch 0002:
> >LGTM (upgrade solved in 0005)
> >
> >Patch 0003:
> >I have no idea.
> >
> >Patch 0004:
> >1)
> >Can you please let it in old school way, for better readability
> >sysupgrade.get_upgrade_state(*upgrade_state_args):
> >
> >sysupgrade.get_upgrade_state('dogtag', 'agent_allow_profile_modify')
> >sysupgrade.set_upgrade_state('dogtag', 'agent_allow_profile_modify' ,True)
> >
> >2)
> >+        conn = ipaldap.IPAdmin(self.fqdn, self.ds_port)
> >+        conn.do_simple_bind(DN(('cn', 'Directory Manager')),
> >self.dm_password)
> >+        conn.modify_s(dn, modlist)
> >+        conn.unbind()
> >
> >IMO You can use:
> >self.ldap_connect()
> >self.admin_conn.modify_s()
> >
> >Patch 0005:
> >LGTM
> >
> >Patch 0006:
> >LGTM
> >
> >Patch 0007:
> >0)
> >  I cannot apply patch
> >
> >error: invalid object 100755 c6c602dc6b4582c24e4ca751ab2f91f8e683dffa
> >for 'install/tools/ipa-upgradeconfig'
> >fatal: git-write-tree: error building trees
> >Repository lacks necessary blobs to fall back on 3-way merge.
> >Cannot fall back to three-way merge.
> >Patch failed at 0007 Import included profiles during install or upgrade
> >
> >
> >1)
> >+        # update `api.env.ca_host` to correct hostname
> >+        # https://fedorahosted.org/freeipa/ticket/4936
> >+        api.env.ca_host = host_name
> >This is something what was already fixed, maybe rebase error?
> >
> >2)
> >+    if setup_ca:
> >+        # ensure profile backend is available
> >+        import ipaserver.plugins.dogtag
> >Must be this import in ipa-server-install?
> >Respectively why is this import required, isn't it initialized by
> >api.finalize()?
> 
> +1, what are you trying to accomplish here?
> 
Moving the `import ipaserver.plugins.dogtag` any earlier - even to
the line before `api.bootstrap(**cfg)` - fails with:

    AttributeError: 'Env' object has no attribute 'ra_plugin'

at ipaserver/plugins/dogtag.py:1271

    if api.env.ra_plugin != 'dogtag':
        ...

Not importing it prior to `api.finalize()` causes the Backend not be
loaded:

    AttributeError: 'NameSpace' object has no attribute 'ra_certprofile'

at /sbin/ipa-server-install:1323, in main:

    api.Backend.ra_certprofile._read_password()

I am not familiar with the plugin loading so I am probably missing
something - if anyone knows how to handle this

> >
> >3)
> >+        service.print_msg("Importing certificate profiles")
> >+        api.Backend.ra_certprofile._read_password()
> >+        if not api.Backend.ldap2.isconnected():
> >+            api.Backend.ldap2.connect(autobind=True)
> >+        ca.import_included_profiles()
> >
> >Why this must be in ipa-server-install?
> >Why is that not in cainstance?
> 
> +1
> 
I should be able to move it back there.

> >
> >4)
> >I'm not sure if these functions can be removed:
> >Honza please take look, if it is safe
> >
> >- def upgrade_ipa_profile(ca, domain, fqdn)
> >
> >-            self.step("set certificate subject base",
> >self.__set_subject_in_config)
> >-            self.step("enabling Subject Key Identifier",
> >self.enable_subject_key_identifier)
> >-            self.step("enabling Subject Alternative Name",
> >self.enable_subject_alternative_name)
> >-            self.step("enabling CRL and OCSP extensions for
> >certificates", self.__set_crl_ocsp_extensions)
> >
> >5)
> >'/usr/share/ipa/profiles/{}.cfg'.format(profile_id), sub_dict)
> >
> >os.path.join(paths.USR_SHARE_IPA_DIR, 'profiles', <profile name>)
> >please use paths from ipaplatform.paths module.
> >Maybe rather create new path in ipaplatform/base/paths
> >USR_SHARE_IPA_PROFILES_DIR = "/usr/share/ipa/profiles"
> >
> >Nitpicks:
> >In several patches:
> >+        except ipalib.errors.PublicError, e:
> >please us except ipalib.errors.PublicError as e
> >
> 
> 6) IPA currently uses the caIPAServiceCert profile. Why don't you import
> this profile and instead create a new DefaultService profile?
> 
DefaultService is exactly this profile - but now IPA owns it, not
Dogtag (https://fedorahosted.org/freeipa/ticket/4002).  I called it
`DefaultService' because users will now be seeing and typing profile
IDs, and I thought `caIPAserviceCert' was a bit unfriendly.

Happy to change it back to `caIPAserviceCert' if that is the
consensus.

Thanks for your reviews!
Fraser

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