[Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP

[Rob Crittenden]

Jan Cholasta wrote:
> Hi,
> 
> the attached patches implement
> <https://fedorahosted.org/freeipa/ticket/3259> and
> <https://fedorahosted.org/freeipa/ticket/3520>.
> 
> This work depends on my patches 241-253 and 262-266
> (<http://www.redhat.com/archives/freeipa-devel/2014-June/msg00276.html>).
> 
> Note that automatic distribution of CA certificates to IPA systems is
> not implemented yet (it's planned for IPA 4.2, see
> <https://fedorahosted.org/freeipa/ticket/4322>), so /etc/ipa/ca.crt,
> /etc/pki/nssdb, /etc/dirsrv/slapd-REALM and /etc/httpd/alias are updated
> *only* during client/server install.

267

What is the purpose of this change? Won't this cause no certificates to
be exported in the default case?

diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py
index 61a954f..3cd7a53 100644
--- a/ipaserver/install/certs.py
+++ b/ipaserver/install/certs.py
@@ -463,7 +463,7 @@ class CertDB(object):
            do that step."""
         # export the CA cert for use with other apps
         ipautil.backup_file(self.cacert_fname)
-        root_nicknames = self.find_root_cert(nickname)
+        root_nicknames = self.find_root_cert(nickname)[:-1]
         fd = open(self.cacert_fname, "w")
         for root in root_nicknames:
             (cert, stderr, returncode) = self.run_certutil(["-L", "-n",
root, "-a"])

Or are you separating out issuing CA from the rest of the chain?

268 ACK

269 ACK

270

If a user has their own CA installed, such as the case where they used
ipa-server-certinstall, this will break it.

What can be in the other set? Docs are needed.

You potentially add a bunch of certs with no trust, what is the purpose?

271

ipaSecretKey isn't mentioned on
http://www.freeipa.org/page/V4/CA_certificate_renewal . What is it and
does it need to be added now?

272 ACK

273 ACK

274 ACK

275 ACK

276

There isn't any error handling around the ASN.1 decoder. Is that wise?

There should be unit tests

277

There should be unit tests

278

When the certificate must be passed in as DER can you use dercert as the
argument name so it's clear what is needed?

In update_ca_cert() and get_ca_certs() should the values for trust be
case insensitive?

This breaks the convention where attribute names are always lower-case.

I can't really grok add_ca_cert(). You are adding certs but removing
values? Is this un-setting the list of trusted CA's maybe?

There isn't a single comment in this file beyond the header.

279

Looks ok

280

Why add the chain with no trust?

281 / 282

What is the difference between add_cert and import_cert other than
passing in trust and not having the db password? Do we even need
add_single_pem_cert anymore?

283 / 284

In __import_ca_certs() I assume the pass is there for NotFound for the
case of creating a replica with an older master that doesn't have these?
How is SSL trust setup then?

This code looks awfully similar.

285

ACK

286

It looks ok, just one wild thought. If writing the certificate fails and
you go to clean up by removing ca_file, what if that removal fails, for
the same reason the write failed, SELinux for example?

287

If this weren't addressed in a later patch I've have dinged you on the:

+    return [cert]

There are some places where you pluralize the variables (certs) and
others where you do not (ca_cert, existing_ca_cert).

288

Is a rawcert a dercert? I'd use that name instead for consistency.

normalize_certificate() can raise exceptions. Those should be handled in
write_certificate_list()

289

ACK

290

This can be dangerous because if another database is opened in the
process things may fail. Additional warnings and red flags should be
provided, or the context should be compared to known places this will
blow up (e.g server).

291

You use a temporary database to make cleanup easier I suppose? Did you
test this in enforcing?

292

Need unit tests

293

Why the fixme for the x509 import?

Isn't this changing already published API for
[insert|remove]_ca_cert_into_systemwide_ca_store?

294

Can you change the old occurances of

 cafile = config.dir + "/ca.crt"

to use CACERT instead?

Bad case in exception, "Ca cert file is not available". Is there any
additional information we can provide, like what to do about it and
where we looked?

You actually remove one occurrence of CACERT and replace it with a
hardcoded path, is that on purpose?

---

I still need to do functional testing and will probably take another
pass the changes through but this patchset generally looks ok.

rob