Refactor Crypto APIs to have a provider scheme #592
Conversation
|
Martin, here's my first whack at a CryptoProvider. OpenSSL is working, I think wincrypto works as well but since there is no default when wincrypto is used the tests are currently broken. But figured it'd be good for you to see if this was a good approach, or if there is anything I should change. I can do a ... For the tests, I was wondering what your opinion was on injecting the CryptoProviderId into tests that need a cryptoprovider, and executing the test with each provider. Do you have a preferred tool or method to run the same test with an injected value (rstest?, something else?). Ideally we'd enumerate the CryptoProviderId and run the tests with each. |
| return CryptoProviderId::OpenSslWithSha1Crate; | ||
| #[cfg(feature = "openssl")] | ||
| return CryptoProviderId::OpenSsl; | ||
| panic!("No default for CryptoProviderId!") |
There was a problem hiding this comment.
don't you need WinCrypto here as well as a default I I compile with only Wincrypto?
There was a problem hiding this comment.
Not from what @algesten indicated he wanted the behavior to be based on how ureq3 was done with nativetls... we should never default to Wincrypto. Or so that's what I understood.
|
I'm away for the weekend so i haven't been able to process this PR yet. Will get to it next week. |
|
Conceivably the Cert set in RtpConfig and the CryptoProvider set in RtpConfig don't have to match, I think allowing that would be a foot gun.... basically... nothing in the CryptoProvider is stateful, so you should be able to interchange cryptoproviders at any time. The Cert itself creates the DtlsContext so those are guaranteed to be compatible, and the DtlsContext manages the Dtls session, so it's all gauranteed to use the same provider. If SRTP happens to use a different provider, there isn't a reason it wouldn't work. So maybe instead of setting the cryptoproviderid.. you set a cryptoprovider in RtpConfig. We have our set of 3 standard providers (OpenSSL, OpenSSL w/ SHA1, Wincrypto). In theory people can provide their own provider, or mix and match providers (like OpenSSL w/ SHA1 is essentially OpenSSL with the SHA1 function replaces). But maybe I'm overthinking this. |
I think it would be reasonable if the default could be set / overridden via a build-time cfg like Also, I just wanted to mention that having no crypto-provider set needs to work as well (full disclosure, I didn't give this a full-review so you may very well cover this anyway). We only use the |
Having no crypto provider will work, however, ice_agent needs a provider for the sha1_hash.. we could have a sha1 only provider.. basically all the other function points in the provider panic or otherwise return an error. |
|
Thinking about this some more.. maybe we should get rid of the cryptoprovider id... and jsut make cryptoproviders be potentially defined outside of str0m... str0m just defines the Provider struct, and the associated traits. WinCrypto can then be it's own crate that depends on Str0m .. and when you use RtcConfig you pass in the CryptoProvider that WinCrypto defines. For the SHA1 case.. we just have a crypto provider with sha1 has defined, and the other crypto functions set to panic/fail. but basically, I think the cryptoproviders can all be defined as exported constants, then passed in. We can leave the SSL, SHA1Only, and SSLwSHA1 in str0m itself since they're already there, and can provide default behavior. I think other than a cryptoprovider, the only other touch point we have are for those Error Enums,but we can just have a generic CyptoProviderError(String), defined by Str0m, we can still leave OpenSSL with it's own, since it's bulit in, but this would allow more complete flexibility for the crypto provider and potneital future providers. |
|
I would like to understand why we need to make these much changes with traits, dynamic dispatch etc? From my perspective, we already have a structure for abstracting the crypto, and this seems like an abstraction on the abstraction. Can we take a step back and discuss what the PR tries to solve? |
|
I might need to take a stab at what I'm thinking myself to understand how we end up with the need for |
|
Starting to see the problem. Our current abstraction kind of forces us to feature flag on the inside of stuff like |
CryptoProvider is a struct which provided functions for performing the various ciphers and hashes Str0m needs. Additionally, it can produce a DtlsIdentity, which in run can be used to create one or more DtlsContext.
CryptoProvider is just function pointers, so it can be cheaply copied as needed. DtlsIdentity holds the "DTLS certificate" instance today, and can then create one or more DTLS Contexts from it. Each DTLS Context manages a DTLS session.
CryptoProvider <- All new thing.
DtlsIndentity <- A trait but the analog was DtlsCertInner
DtlsContext <- A trait but the analog was DtlsInner
Changes in addition to providing a CryptoProvider in many places: