Allow full customisability of IPA CA subject DN #245

frasertweedale · 2016-11-16T11:08:13Z

This patchset adds full customisability of CA subject DN apart from
subject base, via the ipa-server-install --ca-subject option. It
also renames ipa-server-install --subject option to
--subject-base, and adds --ca-subject and --subject-base
options to ipa-ca-install.

Earlier version of this patchset was previously reviewed by
@jcholast on freeipa-devel:
https://www.redhat.com/archives/freeipa-devel/2016-August/msg00570.html

All review items have been addressed, except for item 9. The
suggestion will not work because a fair bit of code besides
what's in ipaserver.install.ca requires knowing the CA Subject DN
and/or subject base. So the defaults must be applied close to the
"entry points".

I also carved a few smaller commits out of the main patch (but it is
still pretty big and hairy to review).

https://fedorahosted.org/freeipa/ticket/2614

tiran · 2016-11-16T11:26:56Z

install/tools/man/ipa-ca-install.1

+.TP
+\fB\-\-subject\-base\fR=\fISUBJECT\fR
+The subject base for certificates issued by IPA (default O=REALM.NAME)
+.TP


Please add a comment that the order of RDNs is in LDAP order, not in X.509 order. It might cause some confusing. Users (like me) expect the order of RDNs like in OpenSSL or Firefox.

https://fedorahosted.org/freeipa/ticket/6455

tiran · 2016-11-16T11:27:17Z

install/tools/man/ipa-server-install.1

+The CA certificate subject DN (default CN=Certificate Authority,O=REALM.NAME)
+.TP
+\fB\-\-subject\-base\fR=\fISUBJECT\fR
+The subject base for certificates issued by IPA (default O=REALM.NAME)


tiran · 2016-11-16T11:28:31Z

ipaserver/install/ca.py

+    'givenname', 'initials', 'generationqualifier', 'dc', 'mail', 'uid',
+    'postaladdress', 'postalcode', 'postofficebox', 'houseidentifier', 'e',
+    'street', 'pseudonym', 'incorporationlocality', 'incorporationstate',
+    'incorporationcountry', 'businesscategory',


OpenSSL has slightly different short names (email instead of e).

I don't think it's an issue. I didn't change any of them, anyhow - so if it is a problem we can address in another ticket / commit.

I wonder if we should mention the RDNs in the man page. After all OpenSSL is a de-facto standard.

frasertweedale · 2016-11-17T00:10:25Z

Added a new commit to add DNs-in-ldap-order comments.

tiran · 2016-11-18T10:53:16Z

flake8 violation: ./ipaserver/plugins/migration.py:750:80: E501 line too long (83 > 79 characters)

frasertweedale · 2016-11-20T23:19:55Z

@tiran I haven't a clue about that pep8 error... my commits don't even touch that file.

tiran · 2016-11-21T08:31:11Z

@frasertweedale I don't have a clue, either. Let's ignore the message.

HonzaCholasta · 2016-11-25T09:59:48Z

install/tools/ipa-ca-install

+                      default=None,
+                      help=(
+                          "The certificate subject base "
+                          "(default O=<realm-name>)"))


IMO this deservers a separate "add missing --subject-base to ipa-ca-install" commit, because the option should have been there since ipa-ca-install gained the ability to install CA master.

As you wish. PR updated.

HonzaCholasta

Besides inline comments:

--subject-base and --ca-subject should be forbidden in ipa-server-install in CA-less mode.
--subject-base and --ca-subject should be forbidden in ipa-ca-install when installing a CA replica.
The ServerReplicaInstall class needs an update (rename subject to subject_base, add ca_subject = None).
We should stop updating ipaCertificateSubjectBase and certmap.conf in CA-less mode (perhaps in other PR).

HonzaCholasta · 2016-12-12T14:35:41Z

install/tools/ipa-ca-install

+        0,
+        re.MULTILINE)
+    with open(certmap_filename, 'w') as f:
+        f.write(certmap_conf)


Please re-use DSInstance.__certmap_conf(). We don't need any more duplicate-but-not-quite-copy-paste code in installers.

OK, no worries cobber.

HonzaCholasta · 2016-12-12T15:04:00Z

install/tools/ipa-ca-install

+    set_subject_base_in_config(realm, dm_password, subject_base)
+
+    # sysupgrade
+    sysupgrade.set_upgrade_state('certmap.conf', 'subject_base', subject_base)


I would really like if we stopped putting subject base in sysupgrade and update DSInstance.find_subject_base(), which is the only user of it, to always read subject base from LDAP.

You are already touching find_subject_base(), so I'm assuming this change would be OK in this PR as well.

I'm wary of the greater scope of that change (hence I filed a separate ticket for it).

I really had no choice but to remove the certmap.conf lookup from find_subject_base() for this ticket though.

IMO the scope is not big (only 4 places including this one need an update), but OK.

HonzaCholasta · 2016-12-12T15:20:04Z

install/tools/ipa-ca-install

+    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 = six.text_type(DN(x509.load_certificate(cert).subject))


This is a duplicate of code in ca.install_check(). I believe this should in fact be done only in ca.install_check().

Yep, it was done here to calculate ca_subject for reset_subject_config(). Per your other review comment, this will be moved to ca.install() so this code can go away.

HonzaCholasta · 2016-12-12T16:08:01Z

install/tools/ipa-ca-install

+        api.env.realm,
+        options.subject_base,
+    )
+    reset_subject_config(api.env.realm, options.ca_subject)


This should be done from inside ca.install(). The ultimate goal for ipa-ca-install is to be just a thin wrapper around ipaserver.install.ca, so the code will end up there sooner or later. I would prefer sooner ;-)

HonzaCholasta · 2016-12-12T16:14:22Z

ipaserver/install/ca.py

+    ipa_ca_nickname = certdb.get_ca_nickname(realm_name)
+    db = certs.CertDB(realm_name, nssdir=paths.IPA_NSSDB_DIR)
+    cert = db.get_cert_from_db(ipa_ca_nickname)
+    return unicode(DN(x509.load_certificate(cert).subject))


Please do not rely on information from the local filesystem in new code. You should use LDAP whenever possible, because unlike the local filesystem, it is guaranteed to have up-to-date information. Here you can use (off the top of my head):

try: ca_subject = api.Command.ca_show(u'ipa')['result']['ipacasubjectdn'] except errors.CommandError: ca_subject = installutils.default_ca_subject_dn(config.subject_base) return unicode(ca_subject)

Hmm, this gets looked up during install_check, before replica DS is created, so we would have to use a remote_api. Now, what if the master is pre freeipa-4.2 thus lacks the ca plugin?

I don't think your suggested approach works in this case. Shall we stay with reading the CA subject DN from the IPA CA cert, or do you have another suggestions?

Using remote_api here should be OK. If the master does not have ca_show, the default CA subject should be used (hence the try-except in the snippet).

HonzaCholasta · 2016-12-13T07:04:17Z

ipaserver/install/cainstance.py

@@ -330,7 +330,8 @@ def configure_instance(self, host_name, dm_password, admin_password,
                           pkcs12_info=None, master_host=None, csr_file=None,
                           cert_file=None, cert_chain_file=None,
                           master_replication_port=None,
-                           subject_base=None, ca_signing_algorithm=None,
+                           subject_base=None, subject=None,


subject should actually be named ca_subject I guess. Ditto for self.subject and DSInstance.

HonzaCholasta · 2016-12-13T07:23:59Z

ipaserver/install/dsinstance.py

-        certdb = certs.CertDB(self.realm, nssdir=dirname, subject_base=self.subject_base)
+        certdb = certs.CertDB(
+                self.realm, nssdir=dirname,
+                subject_base=self.subject_base, ca_subject=self.subject)


Style nitpick: IMO a list of arguments is much easier on the eyes than a matrix / block of arguments:

# good certdb = certs.CertDB(self.realm, nssdir=dirname, subject_base=self.subject_base, ca_subject=self.subject) # also good certdb = certs.CertDB( self.realm, nssdir=dirname, subject_base=self.subject_base, ca_subject=self.subject) # not so good certdb = certs.CertDB( self.realm, nssdir=dirname, subject_base=self.subject_base, ca_subject=self.subject)

Agreed. (Disagree with first approach though; what if we rename var certdb, module certs or class CertDB and length changes? You don't want to realign N extra lines for all the arguments :)

HonzaCholasta · 2016-12-13T07:25:18Z

ipaserver/install/server/replicainstall.py

@@ -704,6 +709,10 @@ def install_check(installer):
        raise RuntimeError("CA cert file is not available. Please run "
                           "ipa-replica-prepare to create a new replica file.")

+    # look up CA subject name (needed for DS certmap.conf)
+    options.subject = unicode(
+        DN(x509.load_certificate_from_file(cafile).subject))


I think you meant options.ca_subject.

Yes; thanks.

HonzaCholasta · 2016-12-13T07:31:42Z

ipaserver/install/server/install.py

-        if 'ipacertificatesubjectbase' not in entry_attrs:
+        cur_subject_base = entry_attrs.get('ipacertificatesubjectbase', [None])
+        if not cur_subject_base[0] \
+                or DN(cur_subject_base[0]) != DN(subject_base):
            entry_attrs['ipacertificatesubjectbase'] = [str(subject_base)]
            conn.update_entry(entry_attrs)


I think an exception handling-based approach might be better:

entry_attrs['ipacertificatesubjectbase'] = [str(subject_base)] try: conn.update_entry(entry_attrs) except errors.EmptyModlist: pass

That is nicer; will do.

HonzaCholasta · 2016-12-13T08:18:50Z

ipaserver/install/ca.py

+        subject_base = options.subject_base
+    else:
+        ca_subject = lookup_ca_subject(options.realm_name)
+        subject_base = replica_config.subject_base


Rather than duplicating this below in install_step_0() and install_step_1(), it would be better to store the values in options._ca_subject and options._subject_base and reuse them in install_step_0() and install_step_1().

Yep, that works fine.

Hm, actually this does not work fine :(

I cannot set the knob value directly because, in the replica_install case, the ca_subject and subject_base knobs have been replaced by a removed property. (ipalib/install/service.py:51).

I cannot set options._ca_subject (or ._subject_base) and then read it back later because the options.__getattr__ raises AttributeError if the attr name does not correspond to a knob name (ipapython/install/core.py:571).

I find the installer/knob code quite hard to follow... do you have a suggestion? (I certainly agree with your original point that it is preferable to compute the values once only, and cache them).

(Previous comment is incorrect; please disregard)

frasertweedale · 2016-12-14T00:55:54Z

@jcholast: new tickets pertaining to subject_base / certmap.conf config:

do not update ipaCertificateSubjectBase and certmap.conf in CA-less mode
- https://fedorahosted.org/freeipa/ticket/6556
do not set (or look up) subject_base in sysupgrade file
- https://fedorahosted.org/freeipa/ticket/6557

Other review comments will be addressed in due course. Thanks for reviewing.

HonzaCholasta · 2017-01-10T10:13:37Z

--subject-base and --ca-subject are not validated in ipa-ca-install.
Please squash "{ds,ca}instance: rename 'subject' to 'ca_subject'" into "Allow full customisability of IPA CA subject DN".
Please use the correct ticket URL in "Add sanity checks for use of --ca-subject and --subject-base".

Refactor set_subject_base_in_config to use api.Backend.ldap2 instead of a manually created LDAP connection. Also rename the function to have a more accurate name, and move it to 'ipaserver.install.ca' to avoid cyclic import (we will eventually need to use it from within that module). Part of: https://fedorahosted.org/freeipa/ticket/2614

`installutils.load_external_cert` assumes that the IPA CA subject DN is `CN=Certificate Authority, {subject_base}`. In preparation for full customisability of IPA CA subject DN, push this assumption out of this function to call sites (which will be updated in a subsequent commit). Part of: https://fedorahosted.org/freeipa/ticket/2614

The --subject option is actually used to provide the "subject base". We are also going to add an option for fully specifying the IPA CA subject DN in a subsequent commit. So to avoid confusion, rename --subject to --subject-base, retaining --subject as a deprecated alias. Part of: https://fedorahosted.org/freeipa/ticket/2614

Part of: https://fedorahosted.org/freeipa/ticket/2614

For full customisability of the IPA CA subject DN, we will need the ability to update DS `certmap.conf' when upgrading a deployment from CA-less to CA-ful. Extract the existing behaviour, which is private to DsInstance, to the `write_certmap_conf' top-level function. Also update `certmap.conf.template' for substition of the whole CA subject DN (not just the subject base). Part of: https://fedorahosted.org/freeipa/ticket/2614

The ca_enabled_check function is a wrapper around api.Command.ca_is_enabled. When using remote_api (e.g. during installer), ca_enabled_check invokes the *global* api instead of the remote_api. Update ca_enabled_check to explicitly receive an api object from the caller and invoke Command.ca_is_enabled through it. Part of: https://fedorahosted.org/freeipa/ticket/2614

Currently only the "subject base" of the IPA CA subject DN can be customised, via the installer's --subject-base option. The RDN "CN=Certificate Authority" is appended to form the subject DN, and this composition is widely assumed. Some administrators need more control over the CA subject DN, especially to satisfy expectations of external CAs when the IPA CA is to be externally signed. This patch adds full customisability of the CA subject DN. Specifically: - Add the --ca-subject option for specifying the full IPA CA subject DN. Defaults to "CN=Certificate Authority, O=$SUBJECT_BASE". - ipa-ca-install, when installing a CA in a previous CA-less topology, updates DS certmap.conf with the new new CA subject DN. - DsInstance.find_subject_base no longer looks in certmap.conf, because the CA subject DN can be unrelated to the subject base. Fixes: https://fedorahosted.org/freeipa/ticket/2614

Update man pages and help output to indicate that --subject-base and --ca-subject options interpret their arguments in LDAP order. Fixes: https://fedorahosted.org/freeipa/ticket/6455

frasertweedale · 2017-01-11T12:47:15Z

@HonzaCholasta PR updated. Re ticket URL, I think 2614 is the correct one for that commit.

HonzaCholasta · 2017-01-11T12:54:42Z

@frasertweedale, the ticket number is correct, but the URL points to Dogtag Trac.

Print an error and terminate if --ca-subject or --subject-base are used when installing a CA-less master or when performing standalone installation of a CA replica. Part of: https://fedorahosted.org/freeipa/ticket/2614

frasertweedale · 2017-01-11T13:07:05Z

@HonzaCholasta whups! Thanks for clarifying; fixed.

HonzaCholasta · 2017-01-11T14:23:31Z

tiran requested changes Nov 16, 2016

View reviewed changes

frasertweedale force-pushed the feature/2614-enhanced-ca-subject branch from fc7cddf to e3419a4 Compare November 17, 2016 00:00

HonzaCholasta self-assigned this Nov 25, 2016

HonzaCholasta reviewed Nov 25, 2016

View reviewed changes

frasertweedale force-pushed the feature/2614-enhanced-ca-subject branch from e3419a4 to 5c0f866 Compare November 30, 2016 06:32

frasertweedale force-pushed the feature/2614-enhanced-ca-subject branch from 5c0f866 to 382bbff Compare December 12, 2016 12:43

HonzaCholasta suggested changes Dec 13, 2016

View reviewed changes

frasertweedale force-pushed the feature/2614-enhanced-ca-subject branch 2 times, most recently from 606cb7f to 42a3091 Compare December 21, 2016 05:10

frasertweedale added 9 commits January 11, 2017 22:42

Extract function for computing default subject base

b8ed126

Part of: https://fedorahosted.org/freeipa/ticket/2614

ipa-ca-install: add missing --subject-base option

66fba65

Part of: https://fedorahosted.org/freeipa/ticket/2614

Indicate that ca subject / subject base uses LDAP RDN order

2a5d595

Update man pages and help output to indicate that --subject-base and --ca-subject options interpret their arguments in LDAP order. Fixes: https://fedorahosted.org/freeipa/ticket/6455

frasertweedale force-pushed the feature/2614-enhanced-ca-subject branch from 42a3091 to 4f98a26 Compare January 11, 2017 12:46

Add sanity checks for use of --ca-subject and --subject-base

d280924

Print an error and terminate if --ca-subject or --subject-base are used when installing a CA-less master or when performing standalone installation of a CA replica. Part of: https://fedorahosted.org/freeipa/ticket/2614

frasertweedale force-pushed the feature/2614-enhanced-ca-subject branch from 4f98a26 to d280924 Compare January 11, 2017 13:14

HonzaCholasta added the ack Pull Request approved, can be merged label Jan 11, 2017

HonzaCholasta closed this Jan 11, 2017

HonzaCholasta added the pushed Pull Request has already been pushed label Jan 11, 2017

frasertweedale deleted the feature/2614-enhanced-ca-subject branch January 12, 2017 00:45

Allow full customisability of IPA CA subject DN #245

Allow full customisability of IPA CA subject DN #245

Conversation

frasertweedale commented Nov 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasertweedale commented Nov 17, 2016

tiran commented Nov 18, 2016

frasertweedale commented Nov 20, 2016

tiran commented Nov 21, 2016

HonzaCholasta Nov 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasertweedale commented Dec 14, 2016

HonzaCholasta commented Jan 10, 2017

frasertweedale commented Jan 11, 2017

HonzaCholasta commented Jan 11, 2017

frasertweedale commented Jan 11, 2017

HonzaCholasta commented Jan 11, 2017

HonzaCholasta Nov 25, 2016 •

edited