Serf's SSL callback doesn't allow retrieving the complete certificate chain

[GoogleCodeExporter]

What steps will reproduce the problem?
1. implement a serf_ssl_need_server_cert_t callback 
2. connect to a server with a certificate that can't be verified

What is the expected output? What do you see instead?

In the callback, I would expect being able to retrieve the certificate chain as 
passed on by the server, so that I can verify it myself, for instance against a 
system certificate store. Instead, I only get the server certificate and have 
no access to any intermediary CA certificates send by the server.

What version of the product are you using? On what operating system?

trunk, OS  X

Original issue reported on code.google.com by frimm...@gmail.com on 14 Sep 2010 at 8:39

[GoogleCodeExporter]

Here's a tentative patch to add the ability to retrieve server certificate 
chains to serf. This adds a new callback to override certificate validation.

* serf_bucket_types.h
   (serf_ssl_need_server_cert_chain_t): New type.
   (serf_ssl_server_cert_chain_callback_set): New function to register a serf_ssl_need_server_cert_chain_t callback.

* buckets/ssl_buckets.c
   (serf_ssl_context_t): Add members to store the new callback.
   (export_certificates): New function. Exports a certificate chain to Base64.
   (validate_server_certificate): Get the certificate chain if a ssl_need_server_cert_chain_t callback is set.
   (serf_ssl_server_cert_chain_callback_set): New function.

* test/serf_get.c
   (print_cert_chain): New function. Prints the certificate chain retrieved from the server.
   (conn_setup): Register the print_cert_chain callback.

Regards,
Pedro

Original comment by muzzleha...@gmail.com on 3 Nov 2010 at 7:27

Attachments:

• serf_cert_chain.diff

[GoogleCodeExporter]

I'm likely going to add this feature to an upcoming serf 1.1.x release. 
(sometime in the next week or so)

Original comment by gstein on 20 Mar 2012 at 9:20

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Is the patch above usable? If so, I have some free cycles to clean it up and 
get it to work with serf-trunk.

Original comment by muzzleha...@gmail.com on 20 Mar 2012 at 9:58

[GoogleCodeExporter]

I just took a quick look at it. I'd request several changes:

* have a new API that sets *both* certificate callbacks, and a single user_data 
value. NULL can be passed for either/none of the two callbacks.

* I'm not sure why the callback type has "need" in the name. The callback isn't 
providing anything. (and yes, serf_ssl_need_server_cert_t seems to be a 
misnomer)

* it seems the chain should be declared: const serf_ssl_certificate_t * const * 
chain. If the callback wants a base64 copy of the cert, it can use 
serf_ssl_cert_export() manually. The extra const means the callback can't 
monkey with the points in the parameter.

Does the above sound reasonable? Assuming so, then I can review/apply any 
updated patch you could provide.

Thanks!

Original comment by gstein on 20 Mar 2012 at 10:13

[GoogleCodeExporter]

Ok, I'll update the patch and follow up on the dev mailing list.

Original comment by muzzleha...@gmail.com on 21 Mar 2012 at 12:21

[GoogleCodeExporter]

In the Apache OpenOffice project I have applied this patch already.  I made 
some additional changes that you might or might not be interested in (please 
keep in mind that I am new to this code, so maybe some/all of the changes may 
not be necessary):

- Added the the length of the cetificate chain as additional parameter to the 
serf_ssl_need_server_cert_chain_t callback, so that the called back code does 
not have to rely on implicit knowledge about the chain.

- Added serf_ssl_server_cert_chain_callback_set to build/serf.def to export 
that symbol.

- Proper initialization of some of the callback pointers.

- validate_server_certificate(...) sets cert_valid (the return value) to 1 when 
the server_cert_chain_callback is in use but is not called (it is called only 
once per chain.)

The patch (of the patch) can be found here:
http://svn.apache.org/viewvc/incubator/ooo/trunk/ext_libraries/serf/serf-1.0.0.i
ssue68b.patch?view=log

Original comment by andrefis...@googlemail.com on 21 Mar 2012 at 8:52

[GoogleCodeExporter]

The length should not be required, as the chain's last element is NULL. I'm 
fine with using either approach: NULL-terminated, or a specified length.

The serf.def file is automatically created by dist.sh when it assembles a 
release. You'll need the patch until you upgrade to the serf 1.1 release, which 
includes the functionality (and will have the func in serf.def).

Original comment by gstein on 21 Mar 2012 at 5:54

[GoogleCodeExporter]

[Hum… this ends up on the dev list so I'm keeping the thread here]

Here's the new version of the patch. You probably will want to ignore the 
changes to test/serf_get.c.

* I named the callback  serf_ssl_server_cert_chain_cb_t  though all serf_ssl 
validation callbacks have "need" in the name. Maybe I should put the need back 
for consistency sake.

* Defined a  serf_ssl_server_cert_chain_callback_set  that sets both callbacks. 
I would have named it  serf_ssl_server_cert_callbacks_set  but that looks too 
similar to existing API...

* As you suggested, the chain is now a: const serf_ssl_certificate_t * const * 
chain. 

From the OpenOffice patch:

* The chain is NULL terminated but I added a certs_len parameter to the 
callback, in case the the length is needed before traversing the chain.

* Initialized the callback pointers. Initialized the cert_pw_callback too.

* I did not set cert_valid to 1 if the server_cert_chain_callback is not 
called. I'm not sure how to proceed. 

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.

Original comment by muzzleha...@gmail.com on 22 Mar 2012 at 6:25

Attachments:

• serf_cert_chain2c.diff

[GoogleCodeExporter]

I've applied the patch with one minor change: you only retrieved the chain when 
depth==0. That would break hard when actual failures arose :-P

There are some additional tweaks that I'll apply in followup commits.

Regarding your comments: I kept the test changes (why not?). The "need" is good 
for callbacks that need some kind of data/input. These are for verification. 
The cert validation callback shouldn't have "need" in the name; I think it just 
blindly followed the previous pattern. Agreed on not setting cert_valid -- if 
the callback is not present to validate the chain, then it should not be 
assumed valid!

I'm not so sure about trimming the chain when intermediates fail. Let's move 
that discussion to the list.

Closing this as Fixed (in r1589). Thanks.

Original comment by gstein on 22 Mar 2012 at 10:35

• Changed state: Fixed