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

--
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