feat: Add support for webpki roots

[ospfranco]

Requirements

• I have added test coverage for new or changed functionality
• I have followed the repository's pull request submission guidelines
• I have validated my changes against all supported platform versions

Related issues

Fixes #106

Describe the solution you've provided

As I described in the issue, the usage of raw rustls fails to load root certs in Android, which causes an app crash whenever a TLS connection attempts to be established.

I've added a new feature that allows to use the rustls-native-certs crate to load the native certificates. I've tested the feature in Android and iOS but not on other platforms.

I'm not super well versed in Rust, but I think the fact that it is behind a feature flag should prevent from accidental breakage.

Comments are more than welcome

Describe alternatives you've considered

I don't know if there is any other way to get this to work with hyper-rustls. In our app we are using Reqwest with native-tls and that works fine, but not sure why this crate relies on the low level hyper implementation.

[ospfranco]

Just went through the docs. cargo test passes as I have not changed the base functionality of the crate, however since I introduced a new feature-flag, running the test with the new flag via:

$ cargo test -p launchdarkly-server-sdk --lib --features "native-certs"

Now fails with:

failures:

---- events::processor_builders::tests::processor_sends_correct_headers::none_matcher_missing_expects stdout ----
Added 154 certificates

thread 'events::processor_builders::tests::processor_sends_correct_headers::none_matcher_missing_expects' panicked at launchdarkly-server-sdk/src/events/processor_builders.rs:419:14:

> Expected at least 1 request(s) to:

POST /bulk
x-launchdarkly-tags: (missing)

...but received 0


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- events::processor_builders::tests::processor_sends_correct_headers::some_application_id_abc_application_sha_xyz_into_application_id_abc_application_sha_xyz_expects stdout ----
Added 154 certificates

thread 'events::processor_builders::tests::processor_sends_correct_headers::some_application_id_abc_application_sha_xyz_into_application_id_abc_application_sha_xyz_expects' panicked at launchdarkly-server-sdk/src/events/processor_builders.rs:419:14:

> Expected at least 1 request(s) to:

POST /bulk
x-launchdarkly-tags: application-id/abc:application-sha/xyz

...but received 0




failures:
    events::processor_builders::tests::processor_sends_correct_headers::none_matcher_missing_expects
    events::processor_builders::tests::processor_sends_correct_headers::some_application_id_abc_application_sha_xyz_into_application_id_abc_application_sha_xyz_expects

test result: FAILED. 258 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.04s

I can try to fix the issue, but some guidance would be appreciated, however, I'm not sure if adding a new feature flag is desired by the maintainers.

[ospfranco]

Fixed the issue, but it's because the connector allos for http or https connections, which is IMO not ideal. I would create a http connector only for the tests.

Only missing the last item on the check list. Supported platforms? The CI tests only appear to run against ubuntu, I'm running this on macOS/iOS/Android

[keelerm84]

Looking into this, I am surprised this is needed.

If you look at the hyper-rustls HttpsConnectorBuilder::with_native_roots implementation, it calls the Config::with_native_roots method, which delegates to the same crate you are using here.

Have you tested your changes against the Android platform yet? Do you know why yours work and this fails?

[ospfranco]

Yes, I have tested in Android, that's why I opened the PR.

Edit: Just to be sure, I just re-tested my changes and I can confirm using the .with_native_roots method simply crashes on Android.

[ospfranco]

A per my last comment the issue is that with_native_roots does not work on Android. Therefore the solution is to use webpki roots, which might also be useful on other platforms where the native certificates cannot be retrieved. I've tested this change and it works.

[kinyoklion]

I think it would be nice to have an example string in the description.

[kinyoklion]

Or in a comment.

[kinyoklion]

We've not added the custom cert capability for rust contract tests, right? Not that we need to right now.

[kinyoklion]

nit: Needs another blank line.

[keelerm84]

Thank you for all the time and energy you have invested on this work and discussion. I made some tweaks to try and get this PR ready for merge. After discussing this we think this can be handled using existing functionality.

These certs are baked into the crate and as a result, adding the package would require regular updates. In our experience from managing other SDKs, this can become a maintenance burden both for us and customers. We feel it is better to allow customers to manage their needs for static certs.

Similar to the example I posted on the related issue, please try the following and let us know if this works for you.

    // Build the HTTPS connector
    let connector = HttpsConnectorBuilder::new()
        .with_webpki_roots()
        .https_or_http()
        .enable_http1()
        .enable_http2()
        .build();

    let mut dsb = StreamingDataSourceBuilder::new();
    dsb.https_connector(connector.clone());

    let mut ep = EventProcessorBuilder::new();
    ep.https_connector(connector);

    let config = ConfigBuilder::new(&sdk_key)
        .data_source(&dsb)
        .event_processor(&ep)
        .build()
        .expect("Config failed to build");
    let client = Client::build(config).expect("Client failed to build");

[ospfranco]

Well... that will effectively lock my app to using a compatible version of rustls, right? At some point if I need to upgrade I will come knocking on the door, but I guess it makes little difference. Once this needs to run on a platform that requires the webpki roots, the update pain will be there.

The snippet will work, no reason for it not to. Thanks for the review though! Closing this :)