On 9/22/2011 5:39 AM, Kaspar Brand wrote: > Having it in one patch seems fine to me, but in the end, it's the > PMC members who will vote on backport proposals (IIUC), so it's > their opinion which really matters.
IINM, I believe we as committers all have a vote... that said, I hope you would drop a +1 in the 2.2 STATUS file after the dust settles on this change :-) > For trunk, I would prefer ssl_log_xerror being used, like so: > > ssl_log_xerror(SSLLOG_MARK, APLOG_WARNING, 0, ptemp, s, inf->x509, > "SSL proxy client cert chain verification failed: %s:", > X509_verify_cert_error_string(X509_STORE_CTX_get_error(sctx))); > > Only having the subject DN in the log message sometimes makes it > difficult to unambiguously identify the cert (think e.g. of the > situation when replacing a previously used cert with a freshly > issued one, where both have exactly the same subject DN). Absolutely. I see it is quite a bit more verbose, but definitely provides "enough" information. > For reasons of readability, I would reorder the OpenSSL calls > after X509_verify_cert() somewhat: > ... > > Note that X509_STORE_CTX_get1_chain() may also return NULL > (not under normal circumstances, but still). For reasons of > robustness, we should fail gracefully in this case. > > And for the logging statements, I would again prefer ssl_log_xerror > being used (especially since it's APLOG_DEBUG level). > Sure... no problem - updates per feedback provided below. I can't think what action we *could* take if the chain comes back NULL, so I wrapped the remaining logic within a check that it is not. Since this is where the trunk and 2.2 patches differ, feel free to have a look at the 2.2. patch here: http://people.apache.org/~druggeri/patches/httpd-2.2-SSLProxyMachineCertificateChainFile.patch The only difference worth noting is the usage of the new logging routine you provided in trunk, but not 2.2. trunk suggestion - if this jives, I'll commit later when I have a bit if (!pkp->ca_cert_file || !store) { return; } /* Load all of the CA certs and construct a chain */ pkp->ca_certs = (STACK_OF(X509) **) apr_pcalloc(p, ncerts * sizeof(sk)); sctx = X509_STORE_CTX_new(); if (!sctx) { ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, "SSL proxy client cert initialization failed"); ssl_die(); } X509_STORE_load_locations(store, pkp->ca_cert_file, NULL); for (n = 0; n < ncerts; n++) { int i, res; X509_INFO *inf = sk_X509_INFO_value(pkp->certs, n); X509_STORE_CTX_init(sctx, store, inf->x509, NULL); /* Attempt to verify the client cert */ if (X509_verify_cert(sctx) != 1) { int err = X509_STORE_CTX_get_error(sctx); ssl_log_xerror(SSLLOG_MARK, APLOG_WARNING, 0, ptemp, s, inf->x509, "SSL proxy client cert chain verification failed: %s :", X509_verify_cert_error_string(err)); } /* Clear X509_verify_cert errors */ ERR_clear_error(); /* Obtain a copy of the verified chain */ chain = X509_STORE_CTX_get1_chain(sctx); if (chain != NULL) { /* Dicard end entity cert from the chain */ /* XXX: This is not needed if we collapse the two * checks in ssl_engine_kernel in the future */ X509_free(sk_X509_shift(chain)); if ((i = sk_X509_num(chain)) > 0) { /* Store the chain for later use */ pkp->ca_certs[n] = chain; } else { /* Discard empty chain */ sk_X509_pop_free(chain, X509_free); pkp->ca_certs[n] = NULL; } ssl_log_xerror(SSLLOG_MARK, APLOG_DEBUG, 0, ptemp, s, inf->x509, "loaded %i intermediate CA%s for cert %i: ", i, i == 1 ? "" : "s", n); if (i > 0) { int j; for (j=0; j<i; j++) { ssl_log_xerror(SSLLOG_MARK, APLOG_DEBUG, 0, ptemp, s, sk_X509_value(chain, j), "%i:", j); } } } /* get ready for next X509_STORE_CTX_init */ X509_STORE_CTX_cleanup(sctx); } X509_STORE_CTX_free(sctx); -- Daniel Ruggeri
