[Freeipa-devel] [PATCHES 0001-0007] Profile management
Fraser Tweedale
ftweedal at redhat.com
Wed May 20 10:46:55 UTC 2015
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.
>
> >>
> >>>>>>>>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.
>
> You shouldn't use the deprecated modify_s method of IPAdmin.
>
> This is not very nice:
>
> + sysupgrade.set_upgrade_state(*upgrade_state_args + (True,))
>
> >>
> >>>>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?
>
> >
> >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
>
As well as importing profiles into Dogtag, this uses the IPA
framework to add corresponding certprofile objects in the IPA
database. Hence it must be done after applying LDAP updates.
> >
> >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?
>
> --
> Jan Cholasta
More information about the Freeipa-devel
mailing list