[Freeipa-devel] [PATCH] 0091 Allow full customisability of CA subject name
Jan Cholasta
jcholast at redhat.com
Fri Sep 23 07:28:21 UTC 2016
On 23.9.2016 09:15, Fraser Tweedale wrote:
> 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.
OK.
>
> Thanks for your comprehensive initial review; there is plenty of
> work still to do :)
Right :-)
BTW could you please split the patch into separate "rename --subject to
--subject-base" and "add --ca-subject" parts?
--
Jan Cholasta
More information about the Freeipa-devel
mailing list