httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Ruggeri <DRugg...@primary.net>
Subject Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Date Thu, 22 Sep 2011 20:25:18 GMT
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


Mime
View raw message