Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wincrypto Implementation for Str0m #589

Merged
merged 24 commits into from
Nov 26, 2024
Merged

Conversation

efer-ms
Copy link
Contributor

@efer-ms efer-ms commented Nov 25, 2024

Implement wincrypto feature to use Windows Crypto APIs instead of OpenSSL on Windows platforms

efer-ms and others added 22 commits November 22, 2024 19:25
…of OpenSSL (#8)

Implement new Str0m feature 'wincrypto'.

When enabled, this cause Str0m to use Windows Cryptographic APIs for crypto.
This feature cannot be used in conjunction with 'sha1' not 'openssl' features, as it provides the same functionality.

Windows APIs are accessed via the windows-rs crate. These calls are `unsafe`, so in order to isolate them from
`safe` str0m code, all `unsafe` code is in a separate `str0m_wincrypto` crate. The code in the core `str0m/crypto/wincrypto` is the glue code between Str0m and this str0m_wincrypto crate.

The `str0m_wincrypto` crate is not intended to be a generic crypto crate, and is very much tailored to match str0m's crypto APIs.
… wincrypt aren't enabled. Add default feature flags to clippy
…a bit to mostly only mark the actual unsafe calls rather than entire function bodies.
* MErge with mainline (#10)

* Fix warning

* Add fuzz to ci

* chore: add warning log when exceeding max number of pairs

* Update changelog

* Bump deps

* 0.6.3

---------

Co-authored-by: Marcus Asteborg <[email protected]>
Co-authored-by: Thomas Eizinger <[email protected]>
Co-authored-by: Martin Algesten <[email protected]>
Co-authored-by: Peter Thatcher <[email protected]>

* build after sync with mainline

* remove dupe lines

---------

Co-authored-by: Marcus Asteborg <[email protected]>
Co-authored-by: Thomas Eizinger <[email protected]>
Co-authored-by: Martin Algesten <[email protected]>
Co-authored-by: Peter Thatcher <[email protected]>
@efer-ms
Copy link
Contributor Author

efer-ms commented Nov 26, 2024

@algesten

For being able to have multiple providers at once and reduce the number of touch points for providers, I was thinking of defining a couple of new traits, which will hopefully eliminate a bunch of the #[cfg]s. So you create the correct cert based on the configured provider, from the Cert you can then create a DtlsContext (or multiple with the same certificate), which will, and start the dtls machinery. Basically, the core of this is the same interface DtlsImpl, but added the SRTP cipher/hashes.

Anyways it would look something like:


trait DtlsCertificate: Clone {
	fn fingerprint(&self) -> Fingerprint;
	fn create_context(&self) -> Box<dyn DtlsContext>	
}

trait DtlsContext {
	// Returns the local certificate fingerprint.
	fn local_fingerprint(&self) -> Fingerprint;

	// DTLS session management
	fn set_active(&mut self, active: bool) -> ();
	fn is_active() -> Option<bool>;
	fn is_connected() -> bool;
	fn handle_handshake(&mut self, out_events: &mut VecDeque<DtlsEvent>) -> Result<bool, CryptoError>;
	fn handle_receive(&mut self, datagram: &[u8], out_events: &mut VecDeque<DtlsEvent>) -> Result<(), CryptoError>;
	fn poll_datagram(&mut self) -> Option<DatagramSend>;
	fn poll_timeout(&mut self, now: Instant) -> Option<Instant>;
	fn handle_input(&mut self, data: &[u8]) -> Result<(), CryptoError>;

	// RTP/SRTP ciphers and hashes
	fn create_aes_128_cm_sha1_80_cipher(key, encrypt) -> Box<dyn aes_128_cm_sha1_80::CipherCtx>;
	fn create_aead_aes_128_gcm_cipher(key, encrypt) -> Box<dyn aes_128_cm_sha1_80::CipherCtx>;
	fn srtp_aes_128_ecb_round(key, input, output) -> ();
	fn sha1_hmac(key, payloads) -> [u8;20];
}

Then in new_from_config we create the context appropriate for the configured provider.

RtcConfig would have crypto_provider: Option<CryptoProvider>

enum CryptoProvider {
  #[cfg(all(feature="openssl", feature="sha1"))]
  OpenSslWithSha1Crate,
  #[cfg(feature="openssl")]
  OpenSsl,
  #[cfg(feature="wincrypto")]
  WinCrypto,
}

If not provided, the default for CryptoProvider is OpenSslWithSha1Crate if the sha1 feature is enabled, otherwise it's OpenSsl , which matches current behavior.

The Sha1 swapping I moved into the openssl provider for simplicity, if wincrypto needs it cause it's sha1 is slow too (haven't measured, it can be added in a similar manner).

...

This is a pretty big refactor, I think we should keep it separate from this wincrypto impl. If you agree this is a good direction, I can work on it separately, I guess the question is whether to get this wincrypto in now, then change the configuration stuff in a follow-up... or hole off on getting wincrypto in until we have this refactor in.

We've already started using the wincrypto provider so we can get some mileage on it.

@algesten
Copy link
Owner

Anyways it would look something like...

This plan looks great!

This is a pretty big refactor, I think we should keep it separate from this wincrypto impl.

My spontaneous thought is that it doesn't look that big. I guess we can merge, although having those conditional compilations here and there doesn't spark joy.

@algesten
Copy link
Owner

Let's fix the conflicts and then merge.

@efer-ms
Copy link
Contributor Author

efer-ms commented Nov 26, 2024

Anyways it would look something like...

This plan looks great!

This is a pretty big refactor, I think we should keep it separate from this wincrypto impl.

My spontaneous thought is that it doesn't look that big. I guess we can merge, although having those conditional compilations here and there doesn't spark joy.

Ok.. I'll get started on the refactor right away. I don't want to let the thoughts escape, and take advantage of the short work week in the US.

@algesten
Copy link
Owner

Ok.. I'll get started on the refactor right away. I don't want to let the thoughts escape, and take advantage of the short work week in the US.

It's ok either way. @xnorpx convinced me :)

@xnorpx
Copy link
Collaborator

xnorpx commented Nov 26, 2024

@efer-ms looks like you need to do a manual rebase and force push

@efer-ms
Copy link
Contributor Author

efer-ms commented Nov 26, 2024

@efer-ms looks like you need to do a manual rebase and force push

I'm confused.. github shows no conflicts. What am I missing?

@xnorpx
Copy link
Collaborator

xnorpx commented Nov 26, 2024

@efer-ms looks like you need to do a manual rebase and force push

I'm confused.. github shows no conflicts. What am I missing?

Never mind need to squash and merge it. Let me do that.

@xnorpx xnorpx merged commit 6b81ff1 into algesten:main Nov 26, 2024
25 checks passed
@efer-ms efer-ms deleted the efer/wincrypto branch November 26, 2024 18:12
Comment on lines +27 to +28
#[cfg(not(any(feature = "openssl", feature = "wincrypto")))]
compile_error!("either `openssl` or `wincrypto` must be enabled");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not quite sure why it works if we depend on str0m with default-features = false, features = ["sha1"] but I expected this line to cause problems.

In the str0m repo, cargo check --no-default-features --features sha1 doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants