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?

Thanks,
Fraser

> >
> >Thanks for your reviews!
> >Fraser
> >
> >>--
> >>Jan Cholasta
> 
> 
> -- 
> 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