On Fri, Sep 23, 2016 at 08:51:02AM +0200, Jan Cholasta wrote: > On 25.8.2016 12:08, Jan Cholasta wrote: > > On 22.8.2016 07:00, Fraser Tweedale wrote: > > > On Fri, Aug 19, 2016 at 08:09:33PM +1000, Fraser Tweedale wrote: > > > > On Mon, Aug 15, 2016 at 10:54:25PM +1000, Fraser Tweedale wrote: > > > > > On Mon, Aug 15, 2016 at 02:08:54PM +0200, Jan Cholasta wrote: > > > > > > On 19.7.2016 12:05, Jan Cholasta wrote: > > > > > > > On 19.7.2016 11:54, Fraser Tweedale wrote: > > > > > > > > On Tue, Jul 19, 2016 at 09:36:17AM +0200, Jan Cholasta wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > On 15.7.2016 07:05, Fraser Tweedale wrote: > > > > > > > > > > On Fri, Jul 15, 2016 at 03:04:48PM +1000, Fraser Tweedale > > > > > > > > > > wrote: > > > > > > > > > > > The attached patch is a work in progress for > > > > > > > > > > > https://fedorahosted.org/freeipa/ticket/2614 (BZ 828866). > > > > > > > > > > > > > > > > > > > > > > I am sharing now to make the approach clear and solicit > > > > > > > > > > > feedback. > > > > > > > > > > > > > > > > > > > > > > It has been tested for server install, replica install > > > > > > > > > > > (with and > > > > > > > > > > > without CA) and CA-replica install (all hosts running > > > > > > > > > > > master+patch). > > > > > > > > > > > > > > > > > > > > > > Migration from earlier versions and server/replica/CA > > > > > > > > > > > install > > > > > > > > > > > on a > > > > > > > > > > > CA-less deployment are not yet tested; these will be > > > > > > > > > > > tested over > > > > > > > > > > > coming days and patch will be tweaked as necessary. > > > > > > > > > > > > > > > > > > > > > > Commit message has a fair bit to say so I won't repeat > > > > > > > > > > > here > > > > > > > > > > > but let > > > > > > > > > > > me know your questions and comments. > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > Fraser > > > > > > > > > > > > > > > > > > > > > It does help to attach the patch, of course ^_^ > > > > > > > > > > > > > > > > > > IMO explicit is better than implicit, so instead of > > > > > > > > > introducing > > > > > > > > > additional > > > > > > > > > magic around --subject, I would rather add a new separate > > > > > > > > > option > > > > > > > > > for > > > > > > > > > specifying the CA subject name (I think --ca-subject, for > > > > > > > > > consistency > > > > > > > > > with > > > > > > > > > --ca-signing-algorithm). > > > > > > > > > > > > > > > > > The current situation - the --subject argument which specifies > > > > > > > > the > > > > > > > > not the subject but the subject base, is confusing enough (to > > > > > > > > say > > > > > > > > nothing of the limitations that give rise to the RFE). > > > > > > > > > > > > > > > > Retaining --subject for specifying the subject base and adding > > > > > > > > --ca-subject for specifying the *actual* subject DN gets us > > > > > > > > over the > > > > > > > > line in terms of the RFE, but does not make the installer less > > > > > > > > confusing. This is why I made --subject accept the full > > > > > > > > subject DN, > > > > > > > > with provisions to retain existing behaviour. > > > > > > > > > > > > > > > > IMO if we want to have separate arguments for subject DN and > > > > > > > > subject > > > > > > > > base (I am not against it), let's bite the bullet and name > > > > > > > > arguments > > > > > > > > accordingly. --subject should be used to specify full Subject > > > > > > > > DN, > > > > > > > > --subject-base (or similar) for specifying subject base. > > > > > > > > > > > > > > IMHO --ca-subject is better than --subject, because it is more > > > > > > > explicit > > > > > > > whose subject name that is (the CA's). I agree that --subject > > > > > > > should be > > > > > > > deprecated and replaced with --subject-base. > > > > > > > > > > > > > > > > > > > > > > > (I intentionally defer discussion of specific behaviour if one, > > > > > > > > none > > > > > > > > or both are specified; let's resolve the question or renaming / > > > > > > > > changing meaning of arguments first). > > > > > > > > > > > > > > > > > > > > > > > > > By specifying the option you would override the default > > > > > > > > > "CN=Certificate > > > > > > > > > Authority,$SUBJECT_BASE" subject name. If --external-ca was > > > > > > > > > not > > > > > > > > > used, > > > > > > > > > additional validation would be done to make sure the subject > > > > > > > > > name meets > > > > > > > > > Dogtag's expectations. Actually, it might make sense to always > > > > > > > > > do the > > > > > > > > > additional validation, to be able to print a warning that if a > > > > > > > > > Dogtag-incompatible subject name is used, it won't be > > > > > > > > > possible to > > > > > > > > > change the > > > > > > > > > CA cert chaining from externally signed to self-signed later. > > > > > > > > > > > > > > > > > > Honza > > > > > > > > > > > > Bump, any update on this? > > > > > > > > > > > I have an updated patch that fixes some issues Sebastian encountered > > > > > in testing, but I've not yet tackled the main change requested by > > > > > Honza (in brief: adding --ca-subject for specifying that, adding > > > > > --subject-base for specifying that, and deprecating --subject; > > > > > Sebastian, see discussion above and feel free to give your > > > > > thoughts). I expect I'll get back onto this work within the next > > > > > few days. > > > > > > > > > Update: I've got an updated version of patch almost ready for > > > > review, but I'm still ironing out some wrinkles in replica > > > > installation. > > > > > > > > Expect to be able to send it Monday or Tuesday for review. > > > > > > > Updated patch attached. > > > > > > I expect it will take a while to review, test and get the ACK, but > > > in any case: IMO it should not be merged until after ipa-4-4 branch > > > gets created. > > > > 1) Please fix these: > > > > $ git show -U0 | pep8 --diff > > ./ipaserver/install/cainstance.py:508:13: E128 continuation line > > under-indented for visual indent > > ./ipaserver/install/dsinstance.py:259:5: E303 too many blank lines (2) > > ./ipaserver/install/installutils.py:999:1: E302 expected 2 blank lines, > > found 1 > > ./ipaserver/install/server/common.py:161:80: E501 line too long (101 > > > 79 characters) > > ./ipaserver/install/server/replicainstall.py:93:80: E501 line too long > > (82 > 79 characters) > > ./ipaserver/install/server/replicainstall.py:604:80: E501 line too long > > (81 > 79 characters) > > > > > > 2) Please put 3rd party library (such as six) imports between standard > > library imports and ipa imports. > > > > > > 3) ipa-ca-install should also have the --subject-base and --ca-subject > > options. > > > > > > 4) Please use the original method of getting the configured subject base > > from ipacertificatesubjectbase of the config object rather than > > DSInstance.find_subject_base(), which is horrendous and should in fact > > be obliterated (not necessarily in this patch). > > > > > > 5) You can use "unicode(x509.get_subject(cert))" to get subject name of > > a cert instead of "unicode(x509.load_certificate(cert).subject)". > > > > > > 6) For every removed "options.subject = ..." there should be a > > "options.subject_base ..." added. > > > > > > 7) The subject base read from replica config should be respected, i.e. > > this bit in ipa-ca-install should stay: > > > > - if config.subject_base is None: > > - attrs = conn.get_ipa_config() > > - config.subject_base = attrs.get('ipacertificatesubjectbase')[0] > > > > Also I would move the rest of the "look up CA subject name" to between > > options.ca_cert_file assignment and ca.install_check() call: > > > > else: > > options.ca_cert_file = None > > > > + # look up CA subject name (needed for DS certmap.conf) > > + ipa_ca_nickname = get_ca_nickname(config.realm_name) > > + db = certs.CertDB(config.realm_name, nssdir=paths.IPA_NSSDB_DIR) > > + cert = db.get_cert_from_db(ipa_ca_nickname) > > + options.ca_subject = unicode(x509.load_certificate(cert).subject) > > + > > ca.install_check(True, config, options) > > if options.promote: > > ca_data = (os.path.join(config.dir, 'cacert.p12'), > > > > > > 8) I think we should remove --subject from ipa-server-install man page > > altogether. > > > > > > 9) I don't like that the default values are set in multiple places > > (CAInstance.configure_instance(), CAInstance.configure_replica(), > > KRAInstance.configure_instance(), KRAInstance.configure_replica(), > > ipa-server-install). The defaults should be handled in one place - ca.py > > - and the values (read from configuration or specified by user or > > default) should be passed through arguments to CAInstance/KRAInstance. > > > > > > 10) Nitpick, but the ca_subject_dn argument of CertDB() would be better > > called just ca_subject and be located after subject_base, for > > consistency with the rest of the patch. > > > > Maybe also rename the subject argument of the various CAInstance and > > KRAInstance methods to ca_subject? > > > > > > 11) I see that there was some code which ignored the configured subject > > base. I think the fixes for that should be moved into a separate patch > > and maybe even a separate ticket. > > > > > > 12) The proper way to rename a Knob and keep the old name is to put the > > old name in cli_aliases of the renamed Knob rather than add a new Knob, > > like this: > > > > subject_base = Knob( > > str, None, > > description="The certificate subject base (default > > O=<realm-name>)", > > cli_aliases=['subject'], > > ) > > > > This way you wouldn't be able to issue a warning when --subject is used, > > but that's OK, as we don't do it for any other renamed options too. > > > > > > 13) AFAIK CN is in fact not valid in a subject base, so it should not be > > added to VALID_SUBJECT_ATTRS. > > > > > > 14) NACK on the normalization stuff. It's not really normalization if > > the original value is not equal to the normalized value. Instead of this > > you should validate if the provided subject name is suitable for Dogtag > > and if it isn't, fail and inform the user what the acceptable format is. > > > > > > 15) Subject base setting is not respected for most of our certs. This is > > with --subject-base='O=Test': > > > > $ sudo getcert list | grep subject > > subject: CN=CA Audit,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=OCSP Subsystem,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=CA Subsystem,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=Certificate Authority,O=Test > > subject: CN=IPA RA,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: > > CN=vm-058-193.abc.idm.lab.eng.brq.redhat.com,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > > > subject: CN=vm-058-193.abc.idm.lab.eng.brq.redhat.com,O=Test > > subject: CN=vm-058-193.abc.idm.lab.eng.brq.redhat.com,O=Test > > > > This is most likely because of 5) and 8) combined. > > > > > > 16) Spaces do not work. This is with --subject-base='O=My Organization': > > > > $ ipa config-show | grep 'Subject base' > > Certificate Subject base: O=My > > > > $ sudo getcert list | grep 'subject' > > subject: CN=CA Audit,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=OCSP Subsystem,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=CA Subsystem,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=Certificate Authority,O=My > > subject: CN=IPA RA,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: > > CN=vm-058-193.abc.idm.lab.eng.brq.redhat.com,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > > > subject: CN=vm-058-193.abc.idm.lab.eng.brq.redhat.com,O=My > > subject: CN=vm-058-193.abc.idm.lab.eng.brq.redhat.com,O=My > > > > I blame the normalization. See also 13). > > > > > > 17) CN in subject base does not work. This is with > > --subject-base='CN=Test': > > > > $ sudo getcert list | grep subject > > subject: CN=CA Audit,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=OCSP Subsystem,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=CA Subsystem,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: CN=Certificate Authority,CN=Test > > subject: CN=IPA RA,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > subject: > > CN=vm-058-193.abc.idm.lab.eng.brq.redhat.com,O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM > > > > subject: CN=Test,CN=Test > > subject: CN=Test,CN=Test > > > > See 12). > > > > > > 18) In CA-less topology, ipa-replica-install fails: > > > > 2016-08-25T09:54:09Z DEBUG Starting external process > > 2016-08-25T09:54:09Z DEBUG args=/usr/bin/certutil -d /etc/ipa/nssdb -L > > -n ABC.IDM.LAB.ENG.BRQ.REDHAT.COM IPA CA -a > > 2016-08-25T09:54:09Z DEBUG Process finished, return code=255 > > 2016-08-25T09:54:09Z DEBUG stdout= > > 2016-08-25T09:54:09Z DEBUG stderr=certutil: Could not find cert: > > ABC.IDM.LAB.ENG.BRQ.REDHAT.COM IPA CA > > : PR_FILE_NOT_FOUND_ERROR: File not found > > > > 2016-08-25T09:54:09Z DEBUG Destroyed connection > > context.ldap2_140045224192976 > > 2016-08-25T09:54:09Z DEBUG Starting external process > > 2016-08-25T09:54:09Z DEBUG args=/usr/sbin/ipa-client-install > > --unattended --uninstall > > 2016-08-25T09:54:18Z DEBUG Process finished, return code=0 > > 2016-08-25T09:54:18Z DEBUG File > > "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 171, in > > execute > > return_value = self.run() > > ----8<------ > > File > > "/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py", > > line 1722, in main > > promote_check(self) > > File > > "/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py", > > line 364, in decorated > > func(installer) > > File > > "/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py", > > line 386, in decorated > > func(installer) > > File > > "/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py", > > line 1266, in promote_check > > options.ca_subject = unicode(x509.load_certificate(cert).subject) > > File "/usr/lib/python2.7/site-packages/ipalib/x509.py", line 125, in > > load_certificate > > return nss.Certificate(buffer(data)) # pylint: disable=buffer-builtin > > > > 2016-08-25T09:54:18Z DEBUG The ipa-replica-install command failed, > > exception: NSPRError: (SEC_ERROR_LIBRARY_FAILURE) security library failure. > > 2016-08-25T09:54:18Z ERROR (SEC_ERROR_LIBRARY_FAILURE) security library > > failure. > > 2016-08-25T09:54:18Z ERROR The ipa-replica-install command failed. See > > /var/log/ipareplica-install.log for more information > > > > > > 19) In CA-less topology, ipa-ca-install fails: > > > > 2016-08-25T09:58:21Z DEBUG raw: ca_show(u'ipa', version=u'2.212') > > 2016-08-25T09:58:21Z DEBUG ca_show(u'ipa', rights=False, all=False, > > raw=False, version=u'2.212') > > 2016-08-25T09:58:21Z DEBUG raw: ca_is_enabled(version=u'2.212') > > 2016-08-25T09:58:21Z DEBUG ca_is_enabled(version=u'2.212') > > 2016-08-25T09:58:21Z DEBUG File > > "/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py", > > line 752, in run_script > > return_value = main_function() > > > > File "/sbin/ipa-ca-install", line 309, in main > > promote(safe_options, options, filename) > > > > File "/sbin/ipa-ca-install", line 279, in promote > > install_master(safe_options, options) > > > > File "/sbin/ipa-ca-install", line 232, in install_master > > subject = api.Command.ca_show(IPA_CA_CN)['result']['ipacasubjectdn'] > > > > File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 449, > > in __call__ > > return self.__do_call(*args, **options) > > > > File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 477, > > in __do_call > > ret = self.run(*args, **options) > > > > File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 799, > > in run > > return self.execute(*args, **options) > > > > File "/usr/lib/python2.7/site-packages/ipaserver/plugins/ca.py", line > > 144, in execute > > ca_enabled_check() > > > > File "/usr/lib/python2.7/site-packages/ipaserver/plugins/cert.py", > > line 222, in ca_enabled_check > > raise errors.NotFound(reason=_('CA is not configured')) > > > > 2016-08-25T09:58:21Z DEBUG The ipa-ca-install command failed, exception: > > NotFound: CA is not configured > > > > This is related to 3). > > Bump. > I expect (hope...) to have cycles to push this forward after my PTO next week.
Thanks for your comprehensive initial review; there is plenty of work still to do :) Cheers, Fraser -- 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