Possibly dead code: Botan::PKIX::check_crl_online

[ksanderon]

Hello, I have look into code regarding that how OCSP and CRL is implemented in botan 2.0.1 and it seems that:

1) in src/lib/x509/x509path.* is function:

/**
* Check CRL using online (HTTP) access. Current version creates a thread and
* network connection per CRL access.

* @param cert_path path already validated by check_chain
* @param trusted_certstores a list of certstores with trusted certs
* @param certstore_to_recv_crls optional (nullptr to disable), all CRLs
* retreived will be saved to this cert store.
* @param ref_time whatever time you want to perform the validation against
* (normally current system clock)
* @param timeout for timing out the responses, though actually this function
* may block for up to timeout*cert_path.size()*C for some small C.
* @return revocation status
*/
CertificatePathStatusCodes
BOTAN_DLL check_crl_online(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
                           const std::vector<Certificate_Store*>& trusted_certstores,
                           Certificate_Store_In_Memory* certstore_to_recv_crls,
                           std::chrono::system_clock::time_point ref_time,
                           std::chrono::milliseconds timeout);

However I not fund any place in whole botan where this function is used. For me this function implementation looks like attempt to use http_util to fetch CRL for cert "online" way. Should botan support revocation verification from CRL online(I suppose it should(and just prefer OCSP method), but I'm not expert)? If yes, how it is done if this part of code is never used? Maybe something in x509_path_validate is missed?

2) There is no support for proxy/tunneling in src/lib/utils/http_util, so pull request which will add support for http proxy could be accepted if I will provide this kind of implementation(CONNECT and/or GET/POST as described in rfc 2068 and rfc 2817)? Please correct me If I'm wrong and this is already implemented in other place or should not because of security reasons ;)

[randombit]

However I not fund any place in whole botan where this function is used.

This function is just exposed for application use at the moment. It would be good for the path validation algorithm to try OCSP and then fallback to CRLs if not available, but at the moment it does not. The basic idea of check_crls_online is the application would call x509_path_validate to validate the chain (presumably with a OCSP timeout of zero to disable the OCSP checks), then call check_crl_online and then use PKIX::merge_revocation_status to merge the chain validation and CRL validation outputs.

All of that is low level and fiddly and probably not even documented, so it would be nice if x509_path_validate handled everything if one passed the proper flags.

There is no support for proxy/tunneling in src/lib/utils/http_util, so pull request which will add support for http proxy could be accepted

You are correct there is no support for proxies ATM. If it doesn't add too much additional complexity I'm ok with adding it. Botan is not really intended to be a full HTTP library, http_util is just the bare minimum to retrieve OCSP and CRLs. So I don't want to go too far afield with it.

It might be good to add an interface class (HTTP_Operations) that provides an interface to the relevant HTTP actions, which is passed to the HTTP-using validation functions. Then, implement a subclass using http_util functions, which could be used in the default case. This would make it possible for applications that are already using some (full featured) HTTP library like curl to easily plug it in so botan used it as well. This would also allow mocking HTTP operations for better testing these validation functions, so a big plus there. If you want to do this and need more direction, just ask.

(These two changes are not mutually exclusive.)

[ksanderon]

Yes, I thought exactly about something like this - to extend api and add option for applications to provide their own implementation of HTTP/transport related stuff, which can be used by botan for OCSP/CRL, while as default api implementation could be taken http_util. This way interface will have backward compatibility to current one. For more this feature could allow application implement smarter way for handling connections - for example async, without spawning threads etc.

And as benefit applications could easily support socks4/5, http proxy and other "exotic" stuff.

I will try to prepare draft for this soon.

Little off topic: Regarding other contributions - can I assume, that code modernization/cosmetic refactor could be accepted(if not require more than C++11)? For example const -> constexpr, if reference to instance not needed, function arguments pass by value not by const& if arguments are small, get rid of naked new/delete by smart pointers, fixes for deprecated constructions (for example implicit copy ctors for classes with explicit dctors), etc, etc...?

[souch]

check_crl_online raises several segfault. There is one in the for loop (i should be c):

-         crls[i] = certstores[i]->find_crl_for(*cert_path[i]);
+         crls[i] = certstores[c]->find_crl_for(*cert_path[i]);

And if the CRL URL is not present it takes timeout to return.

I think this function should be refreshed by "copying" how check_oscp_online is done.

[randombit]

check_crl_online raises several segfault.

Eek. Perils of code not covered by test suite :(

[randombit]

And if the CRL URL is not present it takes timeout to return.

This seems strange to me, because the underlying HTTP call should timeout and throw an exception, causing the future to become ready.

[souch]

I think there is another segfault on the same line when copying the crl: crls[i] = certstores[c]->find_crl_for(*cert_path[i]); I did not investigate, though.

This seems strange to me, because the underlying HTTP call should timeout and throw an exception, causing the future to become ready.

I will test it tomorrow, I don't know what happen, but I am quite sure it waits the timeout.

[souch]

By adding some debug log, we can see the func returns after the timeout, and the exception is not raised, the future seems to be in the state "not ready":

in x509_path_validate() i call check_crl_online instead of check_crl:

-         PKIX::check_crl(cert_path, trusted_roots, ref_time);
+         PKIX::check_crl_online(cert_path, trusted_roots, 0, ref_time,
+                                std::chrono::milliseconds(1000));

Some dirty printf in check_crl_online:

      if(future_crls[i].valid())
         {
         try
            {
            printf("check_crl_online valid wait\n");
            //crls[i] = future_crls[i].get();
            std::future_status status = future_crls[i].wait_for(timeout);
            printf("check_crl_online valid waited\n");
            if(status == std::future_status::ready)
               {
               crls[i] = future_crls[i].get();
               printf("check_crl_online valid get\n");
               }
            }
         catch(std::exception&)
            {
            printf("check_crl_online oups\n");
            // crls[i] left null
            }

output using x509_path_x509test test

check_crl_online except place
check_crl_online valid wait
check_crl_online valid waited // appears 1 second after previous line
check_crl_online valid wait
check_crl_online valid waited

[souch]

The problem above (segfaults) has been fixed #1471. Botan::PKIX::check_crl_online still needs tests.