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

Wed Jun 25 11:13:27 UTC 2014

On 16.6.2014 22:36, Rob Crittenden wrote:
> Rob Crittenden wrote:
>> 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?

find_root_cert() returns the certificate chain *including* the 
end-entity certificate. We want only CA certificates here. This is an 
error introduced in the CA-less rewrite.

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

You can't install own CA with ipa-server-certinstall. I did not see 
anything break.

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

The "trusted" set is for trusted CA certs and the "other" set is for 
other CA certs. I will add a comment...

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

"No trust" in this context means "trust only for issuing CA 
certificates". The certs are there for the sole purpose of forming the 
CA chain, so they don't need to be trusted for anything else.

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

This is the schema from 
<http://www.freeipa.org/page/V4/PKCS11_in_LDAP/Schema#Encoded_key_data>. 
It is shared with DNSSEC. I will add a comment.

>>
>> 272 ACK
>>
>> 273 ACK
>>
>> 274 ACK
>>
>> 275 ACK
>>
>> 276
>>
>> There isn't any error handling around the ASN.1 decoder. Is that wise?

Probably not, but it's consistent with the rest of the x509 module - 
none of its functions do error handling, it is up to the caller.

BTW I have started refactoring x509 code, but didn't have time to 
finish. The new code will include error handling.

>>
>> There should be unit tests

Yes.

>>
>> 277
>>
>> There should be unit tests

Yes.

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

OK.

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

It already is in update_ca_cert(), but get_ca_certs() does indeed need 
to be fixed.

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

I wasn't aware there is such a convention, especially so after seeing 
loads of mixed case attribute names all over IPA code.

Anyway, I don't think it matters anymore, the new LDAP code has 
case-insensitive attribute names.

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

It is unsetting ipaConfigString values from entries that should no 
longer have them. I will add a comment.

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

Sorry, will fix.

>>
>> 279
>>
>> Looks ok
>>
>> 280
>>
>> Why add the chain with no trust?

See my comment for patch 270.

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

add_cert adds certificate by value, import_cert loads it from a file. We 
don't 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?

It is for the case where there is no cn=certificates nor cn=CAcert. The 
trust is setup as usual, failed import does not affect it.

>>
>> This code looks awfully similar.

Will fix.

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

Good point, will fix.

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

Right, will fix.

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

It can be both DER or PEM, I used it for consistency with 
write_certificate().

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

Actually they should be handled by the caller, the same way as with 
write_certificate(). (See my comment for patch 276.)

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

This is used only in ipa-client-install (in patch 291) and it does not 
blow up.

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

Yes, it works fine.

>>
>> 292
>>
>> Need unit tests

OK.

>>
>> 293
>>
>> Why the fixme for the x509 import?

Because it's import from ipalib to ipapython.

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

Yes. I don't think this is a public API and it is changed in Tomáš's 
platform code refactoring anyway.

>>
>> 294
>>
>> Can you change the old occurances of
>>
>>   cafile = config.dir + "/ca.crt"
>>
>> to use CACERT instead?

No, config.dir is the directory where the replica info is unpacked, not 
"/etc/ipa".

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

Maybe. This code is cut-and-pasted from install_ca_cert(), I did not 
touch it otherwise.

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

Yes. Currently, the CA certificate from the replica info is copied 
directly to /etc/ipa/ca.crt, but I want to use it only for connecting to 
LDAP and use whatever is in there to create /etc/ipa/ca.crt.

>>
>> ---
>>
>> I still need to do functional testing and will probably take another
>> pass the changes through but this patchset generally looks ok.
>
> Several issues found during testing:
>
> 1. Enrollment from RHEL-5 fails because the entire cert chain is not
> retrieved, only the issuing CA cert.
>
> Joining realm failed: libcurl failed to execute the HTTP POST
> transaction.  SSL certificate problem, verify that the CA cert is OK.
> Details:
> error:14090086:SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate
> verify failed
> Installation failed. Rolling back changes.

I couldn't reproduce this, I have the full certificate chain in 
http://<master>/ipa/config/ca.crt.

>
> 2. Not quite sure the cause, but this is on a replica:
>
> # ldapsearch -x -Z
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> ldap_start_tls: Operations error (1)
>          additional info: SSL connection already established.
> # extended LDIF
> #
> # LDAPv3
> # base <dc=greyoak,dc=com> (default) with scope subtree
> # filter: (objectclass=*)
>
> Same command on initial master doesn't spit out the p11-kit errors.
>
> Get similar errors out of certutil:
>
> # certutil -L -d /etc/httpd/alias
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
> p11-kit: invalid basic constraints certificate extension
>
> Certificate Nickname                                         Trust
> Attributes
>
> SSL,S/MIME,JAR/XPI
>
> GREYOAK.COM IPA CA                                           CT,C,C
> Server-Cert                                                  u,u,u
> CN=External Authority                                        ,,
> ipaCert                                                      u,u,u
>
> Same version of p11-kit on both machines.
>
> p11-kit-trust-0.20.2-1.fc20.x86_64
> p11-kit-0.20.2-1.fc20.x86_64

This does not happen for me. I don't touch basic constraints anywhere in 
the code. Can I see the "CN=External Authority" certificate?

>
> 3. On uninstall the CA's are not removed from /etc/pki/nssdb and
> /etc/httpd/alias in one of my tests (the first one, so I no longer have
> the logs).

Will fix.

>
> 4. This one isn't your issue AFAICT, not sure if you've seen this one:
>
> # ipa-ca-install ...
>
> [snip]
>
> 2014-06-16T18:07:44Z DEBUG stdout=Loading deployment configuration from
> /tmp/tmprI2isq.
>
> 2014-06-16T18:07:44Z DEBUG stderr=Traceback (most recent call last):
>    File "/usr/sbin/pkispawn", line 530, in <module>
>      main(sys.argv)
>    File "/usr/sbin/pkispawn", line 439, in main
>      info = parser.sd_get_info()
>    File
> "/usr/lib/python2.7/site-packages/pki/server/deployment/pkiparser.py",
> line 442, in sd_get_info
>      info = sd.getSecurityDomainInfo()
>    File "/usr/lib/python2.7/site-packages/pki/system.py", line 40, in
> getSecurityDomainInfo
>      info.name = j['DomainInfo']['@id']
> KeyError: 'DomainInfo'
>
> 2014-06-16T18:07:44Z CRITICAL failed to configure ca instance Command
> ''/usr/sbin/pkispawn' '-s' 'CA' '-f' '/tmp/tmprI2isq'' returned non-zero
> exit status 1
> 2014-06-16T18:07:44Z DEBUG   File
> "/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
> line 639, in run_script
>      return_value = main_function()

I haven't seen this one. Can it be reproduced?

>
> 5. This one may be for Tomas, but:
>
> 2014-06-16T19:01:28Z INFO Deleting schedule 2358-2359 0 from agreement
> cn=meTogrindle.greyoak.com,cn=replica,cn=dc\=greyoak\,dc\=com,cn=mapping
> tree,cn=config
> 2014-06-16T19:01:29Z ERROR unable to convert the attribute
> u'nsds5replicalastupdateend' value '0' to type <type 'datetime.datetime'>
> 2014-06-16T19:01:29Z INFO Replication Update in progress: TRUE: status:
> 0 Replica acquired successfully: Incremental update started: start: 0:
> end: 0
> 2014-06-16T19:01:30Z ERROR unable to convert the attribute
> u'nsds5replicalastupdateend' value '0' to type <type 'datetime.datetime'>
> 2014-06-16T19:01:30Z INFO Replication Update in progress: TRUE: status:
> 0 Replica acquired successfully: Incremental update started: start: 0:
> end: 0
> 2014-06-16T19:01:31Z ERROR unable to convert the attribute
> u'nsds5replicalastupdateend' value '0' to type <type 'datetime.datetime'>
> 2014-06-16T19:01:31Z INFO Replication Update in progress: TRUE: status:
> 0 Replica acquired successfully: Incremental update started: start: 0:
> end: 0
> 2014-06-16T19:01:32Z ERROR unable to convert the attribute
> u'nsds5replicalastupdateend' value '0' to type <type 'datetime.datetime'>
> 2014-06-16T19:01:32Z INFO Replication Update in progress: TRUE: status:
> 0 Replica acquired successfully: Incremental update started: start: 0:
> end: 0

I believe this is <https://fedorahosted.org/freeipa/ticket/4350>.

>
> 6. With an external CA install of F-20 vanilla, upgraded to this patch,
> the external CA is not added to /etc/pki/nssdb. On a pure install of
> this patch, it is added. Not sure if we care since the cert is in the
> global cert store.

Yes, that's one of the effects of "automatic distribution to clients is 
not implemented yet".

>
> 7. Something for the future, but I wonder if test test_0002_find_CA in
> ipatests/test_xmlrpc/test_cert_plugin.py should be able to handle
> external CAs.
>
> rob
>

-- 
Jan Cholasta