feat(tls): AWS Libcrypto Support

[jenr24-architect]

Motivation

I want to use aws-lc-rs with tonic.

Solution

adds a feature flag tls-aws-lc in parallel to tls, removed tls feature flag dependency from tls-*-roots features

[djc]

Changes look good to me, thanks! Need to address the CI failures.

[tottoto]

Thanks. I left some comments.

Could you update the documentation regarding the features.

[LucioFranco]

@jenr24-architect could you try to force push again to see if we can get CI to kick off again?

[jenr24-architect]

@jenr24-architect could you try to force push again to see if we can get CI to kick off again?

Does it have to be a force push?? It doesn't look like my last push triggered it

[tobz]

Just thinking out loud: it strikes me as unfortunate that this the implication here is that because you would only need to access the certificate trust store for doing client things, that we should also enable the channel feature, which is also necessary for client things.

Like in my mind, it would somehow be nicer if this was tls-client-native-roots, etc... to better indicate what the feature was affecting.

Anyways, just thinking out loud. None of this is blocking or needs a response. :)

[tottoto]

I think this is a good idea. However, since the scope is different from this pull request, I think it would be good to discuss it in a separate pull request rather than changing them here.

[tobz]

Looks like this needs to be run through cargo fmt and there's the broken intra-doc link stuff to sort out.

[tottoto]

With this change, users can enable both ring and aws-lc-rs, and when they enable both at the same time, users will need to specify which of these they want to use as the default provider. This setting was already an exposed dependency of rustls as an interface for setting the default provider for rustls, but this was not an issue until now as only ring was supported. I think it would be needed to wrap these configuration APIs in tonic so that users can set which crypto provider to use when both are enabled in order to allow us to use rustls as an internal dependency. It also seems necessary to state that setting the default provider using rustls directly is not intended to be stable for tonic.

[jenr24-architect]

With this change, users can enable both ring and aws-lc-rs, and when they enable both at the same time, users will need to specify which of these they want to use as the default provider. This setting was already an exposed dependency of rustls as an interface for setting the default provider for rustls, but this was not an issue until now as only ring was supported. I think it would be needed to wrap these configuration APIs in tonic so that users can set which crypto provider to use when both are enabled in order to allow us to use rustls as an internal dependency. It also seems necessary to state that setting the default provider using rustls directly is not intended to be stable for tonic.

I had thought about this -- I initially had written some code to add ClientTlsConfig::provider(provider: rustls::CryptoProvider) and ServerTlsConfig::provider(provider: rustls::CryptoProvider) so that I could write both a connect_handles_tls_ring and connect_handles_tls_aws_lc integration test, but the scope of that change started to get out of hand.

I would be willing to add that feature, in either this or a separate PR if we're interested in adding that functionality :)

[LucioFranco]

@jenr24-architect looks like there are some more CI failures

[tottoto]

@jenr24-architect

but the scope of that change started to get out of hand.

Makes sense to me. I suppose there are several possible ways to address this, so it seems reasonable to narrow the scope of this pull request.

[tottoto]

@LucioFranco The CI failure does not seem to be caused by this pull request's change. I opened #2021 to address it.

[jenr24-architect]

merged master after #2021 merged, this PR is ready for CI to re-run and to enter the merge queue