[Freeipa-devel] [PATCHES 0001-0007] Profile management
Jan Cholasta
jcholast at redhat.com
Wed May 20 07:00:14 UTC 2015
Dne 20.5.2015 v 07:56 Fraser Tweedale napsal(a):
> On Wed, May 20, 2015 at 07:40:44AM +0200, Jan Cholasta wrote:
>> Dne 19.5.2015 v 13:50 Fraser Tweedale napsal(a):
>>> 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.
>>
>> Yes, Sub-CAs should also be stored there, but certificate identity mappings
>> should work even without CA installed, so they should be stored somewhere
>> else, like cn=etc.
>>
> That makes sense.
>
>>>
>>>>>>
>>>>>>>>>>>> 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.
>>
>> Thanks. What you did is perfectly valid Python, but let's keep the code
>> readable.
>>
> I am a very DRY person :)
>
>>>
>>>>>>
>>>>>>>> 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
>>
>> This is because ipaserver plugins aren't automatically loaded in installers.
>> I guess we can change that, as we alredy use ldap2 anyway.
>>
>> The code responsible for this is API.bootstrap() in ipalib/__init__.py.
>>
> Thanks, I will look into how we can do this better.
>
>>>
>>>>>
>>>>> 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.
>>
>> I would prefer if the name stayed the same, for backward compatibility.
>>
>> On upgrade you should import the original profile instead of the new one
>> bundled with IPA, so that if the admins did some manual modifications, they
>> won't lose them.
>>
> I agree on the point of importing the original profile (changed or
> unchanged).
>
> On backward compatibility, I'm not sure what the issue is. The name
> `caIPAserviceCert' was AFAIK not exposed through IPA.
>
> It was available for direct use in Dogtag but did anyone actually do
> that or is referencing it by the same name something we need to
> continue to support?
Well, I'm not sure, so I would rather stay on the safe side and not
change the name.
>
> Thanks,
> Fraser
>
>>>
>>> Thanks for your reviews!
>>> Fraser
>>>
>>>> --
>>>> Jan Cholasta
>>
>>
>> --
>> Jan Cholasta
--
Jan Cholasta
More information about the Freeipa-devel
mailing list