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

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.

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

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

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

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

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.

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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to