semantics of cert chain callback (was: Issue 68 in serf: ...)
Greg Stein
>...
> If the server_cert_callback is set, the cert_valid can be set to 1 while the
> full chain is not available since the server_cert_callback will handle the
> intermediate/root certificate(s) errors. However, if the
> server_cert_callback is not set, the server_cert_chain_callback could be
> called with a clean error mask, even if the some certificates (with depth >
> 0) were invalid (e.g. Unknown CA). What should be contract?
>
> The server_cert_chain_callback is responsible for (re)validating the entire
> chain?
> The server_cert_callback must be set to get intermediate failures?
> The server_cert_chain_callback must be called when errors occur?
>
> The patch implements the last one; server_cert_chain_callback is passed a
> chain with length 1 (the current cert) if an error is set.
>
>
> Suggestions? Thanks.
I've been thinking on this, and getting my head wrapped around what is
going on here.
I believe the right answer is the following:
apr_status_t chain_callback(userdata, int failures, int error_depth,
chain, chain_len)
The callback will always be invoked for depth==0. The recipient can
make a copy, if it would like (for display in a UI, for example).
Failures *may* be present, and they are indicated in the FAILURES
parameter (like today).
The new ERROR_DEPTH parameter is used to indicate where in the chain
an error has occurred. We *always* pass the full chain. The callback
can examine the single certificate that has been noted as failing,
and/or look at the entire chain.
The return value is defined same as today.
I believe these semantics provide an answer to the original bug report
(access to the chain), and will also provide the chain if an
intermediate fails. Applications can validate individual certificates
(like the callback in 1.0.x), or examine whole chains as they validate
individual certificates (at depth=ERROR_DEPTH).
How does this plan sound?
Cheers,
-g
Pedro Fonseca
On Mar 22, 11:03 pm, Greg Stein <gst...@gmail.com> wrote:
> On Thu, Mar 22, 2012 at 14:26, <s...@googlecode.com> wrote:
> ...
> > chain?
> > The server_cert_callback must be set to get intermediate failures?
> > The server_cert_chain_callback must be called when errors occur?
>
> > The patch implements the last one; server_cert_chain_callback is passed a
> > chain with length 1 (the current cert) if an error is set.
>
> > Suggestions? Thanks.
>
> an error has occurred. We *always* pass the full chain. The callback
> can examine the single certificate that has been noted as failing,
> and/or look at the entire chain.
>
> The return value is defined same as today.
>
complete chain when depth == 0. Thats why I passed the current
certificate if depth > 0. But thats wrong. By the time serf
validate_server_certificate is called the full chain is available.
Regards,
Pedro
> ...
Greg Stein
I've updated the callback in r1598, and ported it over to
branches/1.1.x in r1599.
Could you take a look and maybe try it out? Assuming there are no
problems, then I'll go ahead and cut a 1.1 release next week.
Andre: could you see whether AOO could use the new APIs on the 1.1.x
branch? You should be able to avoid chunked requests by using the new
serf_bucket_request_set_CL(), and this new chain stuff should be able
to replace the local patch that you apply.
Feedback is most welcome!
Cheers,
-g
Pedro Fonseca
No dia 19/04/2012, às 23:32, Greg Stein <gst...@gmail.com> escreveu:
> On Fri, Mar 23, 2012 at 08:38, Pedro Fonseca <muzzl...@gmail.com> wrote:
>> On Mar 22, 11:03 pm, Greg Stein <gst...@gmail.com> wrote:
>> ...
>>> The new ERROR_DEPTH parameter is used to indicate where in the chain
>>> an error has occurred. We *always* pass the full chain. The callback
>>> can examine the single certificate that has been noted as failing,
>>> and/or look at the entire chain.
>>>
>>> The return value is defined same as today.
>>
>> I was assuming that X509_STORE_CTX_get1_chain() would only return the
>> complete chain when depth == 0. Thats why I passed the current
>> certificate if depth > 0. But thats wrong. By the time serf
>> validate_server_certificate is called the full chain is available.
>
> I've updated the callback in r1598, and ported it over to
> branches/1.1.x in r1599.
>
> Could you take a look and maybe try it out? Assuming there are no
> problems, then I'll go ahead and cut a 1.1 release next week.
>
Sure. I'll try to take a look over the weekend.
Regards,
Andre Fischer
We are currently in the process of finishing our first release of Apache
OpenOffice (incubating), so it is not a good time for updating libraries
right now. I will look into this when the release is out.
Anyway, many thanks for your work. The fewer patches we need to use an
external library, the better.
-Andre
Greg Stein
On Apr 20, 2012 3:45 AM, "Andre Fischer" <andrefi...@googlemail.com> wrote:
>
> On 20.04.2012 06:32, Greg Stein wrote:
>...
>> I've updated the callback in r1598, and ported it over to
>> branches/1.1.x in r1599.
>>
>> Could you take a look and maybe try it out? Assuming there are no
>> problems, then I'll go ahead and cut a 1.1 release next week.
>>
>> Andre: could you see whether AOO could use the new APIs on the 1.1.x
>> branch? You should be able to avoid chunked requests by using the new
>> serf_bucket_request_set_CL(), and this new chain stuff should be able
>> to replace the local patch that you apply.
>
>
> We are currently in the process of finishing our first release of Apache OpenOffice (incubating), so it is not a good time for updating libraries right now. I will look into this when the release is out.
>
> Anyway, many thanks for your work. The fewer patches we need to use an external library, the better.
Right. That's my goal, too... reduce your patching, as that implies a need from serf. I hope to address that need.
Good luck with your first AOO release!
Cheers,
-g
Pedro Fonseca
>...
On Apr 20, 2012 3:45 AM, "Andre Fischer" <andrefi...@googlemail.com> wrote:
>
> On 20.04.2012 06:32, Greg Stein wrote:
>> I've updated the callback in r1598, and ported it over to
>> branches/1.1.x in r1599.
>>
>> Could you take a look and maybe try it out? Assuming there are no
>> problems, then I'll go ahead and cut a 1.1 release next week.
>>
Greg Stein
On Apr 22, 2012 2:33 PM, "Pedro Fonseca" <muzzl...@gmail.com> wrote:
>>...
> Hi. It looks OK.
Thanks for a second look!
>
> I wasn't able to test it, but here are some notes (not as thorough as I would like).
>
> /* Borrow the chain to pass to the callback. */
> chain = X509_STORE_CTX_get_chain(store_ctx);
>
> I would rather get the copy of the chain with _get1_chain since get_chain just returns ctx->chain and it feels kind of wrong to manipulate it directly. On the other hand, I don't like needless copies and the code on the serf side looks sane.
Yeah, we're just reading the chain elements, and we're not making other openssl calls which might change it underneath us. Thus it felt safe with less copyihg.
>
> /* If the chain can't be retrieved, just pass the current
> certificate. */
> /* ### can this actually happen with _get_chain() ? */
>
> Humm ...if something goes wrong with X509_verify_cert(), which at some point calls the verification callback registered by serf, the chain can be null. I'm not sure how likely is that to happen, but I guess its not that likely.
>
> I don't have time at this moment to dig into into openssl code (well... the experience was quite painful the last time I did it (-: ).
Haha... I bet. My brief perusal wasn't too fun.
> The relevant code is located in crypto/x509/x509_vfy.c. From my notes a few minor patches ago:
>
> 168: cb = ctx->verify_cb # cb is serf validate_server_certificate
>
> 243: if (ctx->check_issued( # by this time the chain is built. cb can be called for the first time or...
>
> 260: ok = cb(0, ctx) # ...here.
Thanks. I think I'll dig in and take a look. I'd prefer to avoid the complexity and the "fake chain" for a nonexistent condition.
Cheers,
-g