httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kaspar Brand <httpd-dev.2...@velox.ch>
Subject Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Date Thu, 22 Sep 2011 10:39:14 GMT
On 21.09.2011 19:40, Daniel Ruggeri wrote:
> Also - any opposition to including SSL_X509_NAME_to_string as part of
> the backport proposal? I would like to keep the patches consistent. If
> not, would you prefer me to roll it into the
> SSLProxyMachineCertificateChainFile patch or propose it separately?

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.

>         if (X509_verify_cert(sctx) != 1) {
>             int err = 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_dn, X509_verify_cert_error_string(err));
>         }

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

>         chain = X509_STORE_CTX_get1_chain(sctx);
> 
>         /* 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));
> 
>         i = sk_X509_num(chain);
>         pkp->ca_certs[n] = chain;
> 
>         if (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_dn);
>         if (i > 0) {
>             int j;
>             for (j=0; j<i; j++) {
>                 X509_NAME *ca_name =
> X509_get_subject_name(sk_X509_value(chain, j));
>                 char *ca_dn = SSL_X509_NAME_to_string(ptemp, ca_name, 0);
>                 ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "%i: %s", j,
> ca_dn);
>             }
>         }
>     }

For reasons of readability, I would reorder the OpenSSL calls
after X509_verify_cert() somewhat:

          /* clear X509_verify_cert errors */
          ERR_clear_error();

          /* grab a copy of the chain */
          chain = X509_STORE_CTX_get1_chain(sctx);

          /* remove the EE cert from the chain */
          X509_free(sk_X509_shift(chain));

          /* deal with what is left now */
          if ((i = sk_X509_num(chain)) > 0) {
              /* remember the certs in pkp->ca_certs */
          }
          else {
              /* discard "chain" */
          }

          /* get ready for next X509_STORE_CTX_init */
          X509_STORE_CTX_cleanup(sctx);

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

Kaspar

Mime
View raw message