On 9/23/2011 10:07 AM, Kaspar Brand wrote:
> On 22.09.2011 22:25, Daniel Ruggeri wrote:
>> trunk suggestion - if this jives, I'll commit later when I have a bit
> Looks good, just some nits:
>
>> for (n = 0; n < ncerts; n++) {
>> int i, res;
> res is no longer used, AFAICT
Correct - removed
>> 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));
> s/Di/Dis/. As for the XXX, do you mean the idea of having a common
> routine for checking server certs and proxy client certs? That would
> probably go to ssl_engine_init.c as well, as sort of a companion to
> ssl_check_public_cert().
In the proxy client cert callback function in ssl_engine_kernel, each
cert is first checked if it is directly signed by each of the CA's in
the list. If that fails, then we start trying to match by chain. The
comment I added just points out that if we leave the end cert in the
STACK_OF(X509) we will perform the same check twice - once for the
direct issuer check and once again for the first item in the chain
without shifting it off.
Alternatively, we could adjust the callback and init functions to always
build a chain (even if SSLProxyMachineCertificateChainFile is not set)
and check "by chain" by doing the X509_NAME_cmp for each item in the
STACK_OF(X509) in pkp->ca_certs rather than checking the issuer of each
item in pkp->certs. If the new directive is not set, everything would
*essentially* function the same way. To me, they are two ways to do the
same thing, though with the current approach, the verification messages
in startup will not show up unless using the new directive.
... I'm not sure if I explained my thought process well, though, so let
me know if I should elaborate further.
>> else {
>> /* Discard empty chain */
>> sk_X509_pop_free(chain, X509_free);
>> pkp->ca_certs[n] = NULL;
> Strictly speaking, the last assignment isn't necessary, since your
> calloc'ing ca_certs before.
Setting to NULL will be caught by the update Rüdiger put in for 1162103
and will skip all of the new logic in the callback function. IMO, I feel
this way is just a bit cleaner and easier to follow. I can be swayed if
you feel strongly about it, though.
> Style - missing spaces. Kaspar
I'm so bad about this. Corrected also. Thank you very much for
reviewing. I'll wait for feedback before committing and updating 2.2 STATUS.
--
Daniel Ruggeri
