[Freeipa-devel] [PATCHES 0001-0007] Profile management
Martin Basti
mbasti at redhat.com
Fri May 15 12:27:41 UTC 2015
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.
>
>>>>>>> 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.
>
>>> 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()?
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?
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
--
Martin Basti
More information about the Freeipa-devel
mailing list