Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
Comments buried deep inline. Jan Cholasta wrote: 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. OK. 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. Sorry, I wasn't very clear. Imagine the case where the user has replaced the server certificate in Apache with something issued by some other CA that requires that some CA certificates also be installed. This is a not-too-common but seen scenario, mostly to avoid having to trust the IPA CA in browsers. The way I read fix_trust_flags() is that it would remove the trust from unknown CA's which could cause the server to not start. IMHO you should leave alone certs you don't know about. 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. Well, sure, but it isn't doing ASN.1 decoding of raw certs either. A lot can go wrong there. 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. Ok, I won't fight it, but look in ipalib/plugins. The majority of objectclasses/attributes are lower case. Anything otherwise was missed in review. 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. OK 281 / 282 What is the difference between add_cert and import_cert other than
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
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
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
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
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
On 06/12/2014 07:45 PM, Jan Cholasta wrote: ... 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. Honza For 4.0, we will need to come up with manual procedure how to renew the certificates *without* reinstalling the client or server. I think the best way would be to prepare a simple script to renew client/server, something like /usr/share/ipa/ipa-renew-client-certificate /usr/share/ipa/ipa-renew-server-certificate and refer to it in the ipa-cacert-manage man page. People could then pretty easily run those after a cert change, using whatever means their infrastructure uses - puppet, ssh, ... Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
On Fri, 2014-06-13 at 09:05 +0200, Martin Kosek wrote: On 06/12/2014 07:45 PM, Jan Cholasta wrote: ... 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. Honza For 4.0, we will need to come up with manual procedure how to renew the certificates *without* reinstalling the client or server. I think the best way would be to prepare a simple script to renew client/server, something like /usr/share/ipa/ipa-renew-client-certificate /usr/share/ipa/ipa-renew-server-certificate I assume you mean /usr/bin or /usr/libexec/ipa ? and refer to it in the ipa-cacert-manage man page. People could then pretty easily run those after a cert change, using whatever means their infrastructure uses - puppet, ssh, ... -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
On 06/13/2014 02:55 PM, Simo Sorce wrote: On Fri, 2014-06-13 at 09:05 +0200, Martin Kosek wrote: On 06/12/2014 07:45 PM, Jan Cholasta wrote: ... 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. Honza For 4.0, we will need to come up with manual procedure how to renew the certificates *without* reinstalling the client or server. I think the best way would be to prepare a simple script to renew client/server, something like /usr/share/ipa/ipa-renew-client-certificate /usr/share/ipa/ipa-renew-server-certificate I assume you mean /usr/bin or /usr/libexec/ipa ? Right, that's better. I think we do not want to store it in /usr/bin as fully supported scripts as I would feel obliged to keep that scripts supported and around even when automatic renewal is available in FreeIPA 4.2. So maybe /usr/libexec/ipa would be better. and refer to it in the ipa-cacert-manage man page. People could then pretty easily run those after a cert change, using whatever means their infrastructure uses - puppet, ssh, ... ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
Martin Kosek wrote: On 06/13/2014 02:55 PM, Simo Sorce wrote: On Fri, 2014-06-13 at 09:05 +0200, Martin Kosek wrote: On 06/12/2014 07:45 PM, Jan Cholasta wrote: ... 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. Honza For 4.0, we will need to come up with manual procedure how to renew the certificates *without* reinstalling the client or server. I think the best way would be to prepare a simple script to renew client/server, something like /usr/share/ipa/ipa-renew-client-certificate /usr/share/ipa/ipa-renew-server-certificate I assume you mean /usr/bin or /usr/libexec/ipa ? Right, that's better. I think we do not want to store it in /usr/bin as fully supported scripts as I would feel obliged to keep that scripts supported and around even when automatic renewal is available in FreeIPA 4.2. So maybe /usr/libexec/ipa would be better. I guess it depends on what our expectations of user's running this are. If it is basically sample code, then yeah, /usr/share may be ok. If it's something supported we expect some people to run, /usr/[s]bin is probably the place. /usr/libexec is for binaries run by other programs IIRC. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
