On 9/17/2011 6:02 AM, Dr Stephen Henson wrote:
> Yes you need store the returned value and free it with X509_free().
>
> Note also that because you ignore return values of X509_verify_cert() you
> might
> have a situation where the chain is not complete and so deleting the last
> element will remove a non-root CA.
Both suggestions make sense - here is what was just committed to
trunk... I also added logging of verification failures at WARNING level.
Since I was in the file again anyhow, I added logging at DEBUG of what
gets loaded and the order so there is no ambiguity.
...
for (n = 0; n < ncerts; n++) {
int i, res;
char cert_cn[256];
X509_INFO *inf = sk_X509_INFO_value(pkp->certs, n);
X509_NAME *name = X509_get_subject_name(inf->x509);
X509_NAME_oneline(name, cert_cn, sizeof(cert_cn));
X509_STORE_CTX_init(sctx, store, inf->x509, NULL);
res=X509_verify_cert(sctx);
chain = X509_STORE_CTX_get1_chain(sctx);
if (res == 1) {
/* Removing the client cert if verification is OK
* could save a loop when choosing which cert to send
* when more than one is available */
/* XXX: This is not needed if we collapse the two
* checks in ssl_engine_kernel in the future */
X509_free(sk_X509_shift(chain));
}
else {
int n = X509_STORE_CTX_get_error(sctx);
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
"SSL proxy client cert chain verification
failed for %s: %s",
cert_cn, X509_verify_cert_error_string(n));
}
ERR_clear_error();
i = sk_X509_num(chain);
pkp->ca_certs[n] = chain;
if (i == 0 || (res != 1 && i == 1) ) {
/* zero or only the client cert won't be very useful
* due to verification failure */
sk_X509_pop_free(chain, X509_free);
i = 0;
pkp->ca_certs[n] = NULL;
}
X509_STORE_CTX_cleanup(sctx);
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
"loaded %i intermediate CA%s for cert %i (%s)",
i, i == 1 ? "" : "s", n, cert_cn);
if (i > 0) {
int j;
for (j=0; j<i; j++) {
char ca_cn[256];
X509_NAME *ca_name =
X509_get_subject_name(sk_X509_value(chain, j));
X509_NAME_oneline(ca_name, ca_cn, sizeof(ca_cn));
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "%i: %s", j,
ca_cn);
}
}
}
X509_STORE_CTX_free(sctx);
...
--
Daniel Ruggeri
