Refactor crypto feature flags

[algesten]

This is a refactor to make less use of the feature flags openssl and wincrypto for conditional compilation. It plugs in "dummy" impls that panics if the corresponding feature is not turned on. The goal is:

• Able to compile with --all-features (no mutually exclusive feature flags)
• Always default to a specific crypto (currently openssl), even if that fails runtime. This is to ensure consumers make the correct config instead of relying on feature flags to get a desired behavior
• Minimize amount of code behind conditional compilation to aid refactoring

[algesten]

This might be an alternative route to #592

[algesten]

@efer-ms @xnorpx I think this is more the way I'd like to go (open to challenges).

Only the DTLS part is implemented. The next would be to "package up" the SRTP crypto functions under something mirroring the DTLS side.

• Move the "free floating" fns new_aes_128_cm_sha1_80, new_aead_aes_128_gcm and srtp_aes_128_ecb_round to some container enum (like SrtpCrypto).
• Make a DummySrtpCryptoImpl
• Delegate the calls via SrtpCrypto in the same way

[algesten]

I'm happy to keep working on this, or hand it over.

[algesten]

Harder than I thought to pull this refactor off 😰

[efer-ms]

I'm happy to keep working on this, or hand it over.

I'm happy to hand it off to you.

[efer-ms]

So I'll defer to you Martin, but I really thing the whole crypto thing can be isolated. There shouldn't be a need to update anything in str0m in order to provide a new crypto implementation. This would allow for a single compliance test to exist for any provider, and ensure that str0m doesn't get polluted with knowledge over the various crypto providers that may or may not exist.

I used the traits for Dtls/DtlsCert to avoid having to sprinkle #cfg everywhere. It makes it more difficult to implement providers, and to keep them isolated.

I only kept DtlsCert around in mine vs. using the trait directly because I was trying to preserve the existing API, which exposes DtlsCert for reuse, but really you could just provide a DtlsIdentity into rtcconfig, as eliminate that as well. The cost is dynamic dispatch, but in your latest code, you end up having a match between implementations anyways, so why not simply use a dynamic dispatch?

Anyways, I'm okay for you to proceed down your path if it's your preferred approach, otherwise I could make the changes I'm talking about in my branch if you want to see what that looks like.

Basically, I think you can end up with #cfg only cause we have a default provider (openssl and/or sha1).

[thomaseizinger]

FWIW, another approach could be to use global state. That is what rustls does: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html#method.install_default

I haven't researched their decision process on how they ended up with that design so blindly following it may be an act of cargo cult. Nevertheless, I think it is a good data point for how this is handled in the ecosystem. What we could do is:

• Have a CryptoProvider trait that is implemented for various providers
• The crypto module exposes implementations of those
• The CryptoProvider trait has an install function that sets a global variable.
• Anything that needs a certain crypto function just refers to the global state

If we wanted, this could also be split up into multiple crates although I am not sure it is necessary right away. We'd need:

• str0m_crypto to provide the trait + global state
• str0m_openssl for the OpenSSL impl
• str0m_wincrypto for the Win impl
• str0m that pulls it all together and exposes the necessary feature-flags

[algesten]

There shouldn't be a need to update anything in str0m in order to provide a new crypto implementation.

That level of modularity is not something we necessarily need to strive for. It's nice, but I think might come at the cost of more abstraction and code complexity.

I only kept DtlsCert around in mine vs. using the trait directly because I was trying to preserve the existing API, which exposes DtlsCert for reuse, but really you could just provide a DtlsIdentity into rtcconfig, as eliminate that as well. The cost is dynamic dispatch, but in your latest code, you end up having a match between implementations anyways, so why not simply use a dynamic dispatch?

As a fundamental principle, str0m is built trying to avoid dynamic dispatch, especially in expensive places. Now, we have failed on that because the decryption/encryption of SRTP is doing it. I also concede I might be overly worrying about it, because the time to encrypt/decrypt probably overshadows the dispatch with orders of magnitude.

Maybe a point of pride rather than a point of reason.

FWIW, another approach could be to use global state.

Interesting idea. I have considered that approach for ureq, but maybe it's applicable here too. At the very least I should probably unify DTLS and SRTP under one CryptoProvider trait.

[thomaseizinger]

As a fundamental principle, str0m is built trying to avoid dynamic dispatch, especially in expensive places.

As a datapoint, in our application, even str0m's crypto code is only a low, single-digit percentage of CPU usage under high-load. The IceAgent also only does a few hmac-sha1 though so might not be super-representative.

[algesten]

As a datapoint, in our application, even str0m's crypto code is only a low, single-digit percentage of CPU usage under high-load. The IceAgent also only does a few hmac-sha1 though so might not be super-representative.

Yeah, the ICE sha1 are neither here nor there. The SRTP encryption/decryption is in comparison what takes most CPU for the rest of the WebRTC stack.

[xnorpx]

As a fundamental principle, str0m is built trying to avoid dynamic dispatch, especially in expensive places. Now, we have failed on that because the decryption/encryption of SRTP is doing it. I also concede I might be overly worrying about it, because the time to encrypt/decrypt probably overshadows the dispatch with orders of magnitude.

I haven't looked to carefully at the code, but if you can do the decision at compile time and pick one provider for dtls/srtp and one provider for sha-1 at compile time with features and then use static dispatch. I don't think there is ever a case when I want to switch between Wincrypto and OpenSSL runtime. Or pure rust implementation vs openssl in the future.

[thomaseizinger]

I got nerd-sniped into having a crack at this myself: #597