support for CURLOPT_SSL_CTX_FUNCTION / ssl_ctx

[yoav-steinberg]

In my app I need some custom tweaking of the underlaying OpenSSL context. Curl povides this via the CURLOPT_SSL_CTX_FUNCTION option and this seems to be implemented in curl-rust's Handler trait. What would be the best way to patch Isahc to expose this so I can add my own tweaks to the callback?
Perhaps this can be added to Isahc somehow?

[sagebind]

Good question. This seems like a more generalized version of #89. Generally I've been hesitant to add anything to the API that relies on specific implementation details such as the SSL/TLS engine being used. Since libcurl could be linked dynamically, we won't know for sure which SSL/TLS engine will even be in use until runtime.

While I'm not a fan of exposing many unsafe APIs, trying to add a safe wrapper around this would be complicated, and an easy footgun if it turns out OpenSSL is not used in the final application. I'm not sure how I feel about exposing this either, since it could easily be misused to cause security vulnerabilities.

[yoav-steinberg]

@sagebind Thanks for the reply. I agree with the risks exposing this can have. Both the risk of abuse and the abstraction of Curl's TLS layer make it a dangerous feature. Nonetheless I'd like to make two points for adding this:

1. As hinted by Support in-memory client / CA certificates #89, when using TLS for anything other than simple HTTPS with known, pre-instaledl CA's for public web access, configuration and tweaking are required. In my specific use case I have no way around this. One way to work around the risks this entails is to have some optional feature for "configurable-tls" and by default not build all these tweaking options.
2. Regarding Curl's underlying TLS layer - using the curl::Version::get().ssl_version() method in the callback seems to be the standard way to detect what's running down there and avoiding potential issues if the engine isn't what you expect it to be (OpenSSL in my case). I know this is (very) far from rust level security, but it is possible, and it's indeed what I had in mind.

Assuming these aren't convincing for such a change, I'm still very much tempted to fork and and my own patch since I have no way around it and Isahc is perfect for me in every other respect. Playing around with the code I found it a bit tricky create a configurable optional callback for this and having an API for it in the Configurable trait. Any help in how to best add this would be appreciated as I'm still very new to rust.

[sagebind]

One way to work around the risks this entails is to have some optional feature for "configurable-tls" and by default not build all these tweaking options.

That's a great idea! I did not think of that. If we can put this beind a feature then I'm less concerned about adding this; though I'd still want to put appropriate warning labels on the unsafety of the API.

I found it a bit tricky create a configurable optional callback for this and having an API for it in the Configurable trait.

I agree that the way configuration is done right now requires some fiddling. I hope to refactor the internals at some point (for performance reasons) but the trait will stay the same.

I might use a trait instead of a plain callback, and then wrap it in a struct. Maybe something like this? (Names might need tweaked.)

struct CustomTls {
    configurer: Box<dyn CustomTlsConfigurer + Send + Sync + 'static>,
}

pub trait CustomTlsConfigurer {
    unsafe fn configure(&self, ctx: *mut c_void) -> Result<(), Error>;
}

pub trait Configurable {
    unsafe fn danger_custom_tls_config<T>(self, configurer: T)
    where
        T: CustomTlsConfigurer + Send + Sync + 'static,
    {
        self.configure(CustomTls {
            configurer: Box::new(configurer),
        })
    }
}

Then in HttpClient, just pull out CustomTls if any is set, and then pass an Option<CustomTls> to the handler constructor so that it can invoke it from ssl_ctx.

[yoav-steinberg]

Thanks! This approach seems to work. I'll try to submit a PR soon (unless you're planning to :).