Standalone ICE agent?

[thomaseizinger]

I am back with another inquiry! :)

Currently, I am trailing usage of str0m's IceAgent in a project that just needs ICE but nothing else around WebRTC. In particular, we don't want to pull in openssl as a dependency. I have a few small patches locally that publicly export a handful APIs around the ice module. However, I also have one that pretty much comments out everything that deals with openssl which is just a hack for now.

I would like to get some opinions on what a collaborative and useful way forward is. I think a SANS-IO & compliant ICE implementation is something really useful so I don't really want to fork str0m longterm. The ice module also depends on the io and parser module which makes extracting it not quite straight-forward. Additionally, splitting up str0m into multiple crates creates an additional maintenance burden.

I think extracting a dedicated str0m-ice crate (name to be bike-shedded) would make the most sense:

• The dependency on parser could be avoided by removing Candidate::from_sdp_string. This one is actually a circular dependency so could be argued shouldn't exist in the first place.
• The dependency on io is a bit trickier. To do message parsing, stun_codec could be used. I've had good experiences with it. Alternatively, something like str0m-stun would have to be introduced that handles message parsing.
• The rest of IO, i.e. all the SANS-IO modelling would either also have to go into a dedicated crate or perhaps inlined into str0m-ice?

Curious to hear your thoughts!

[algesten]

Hi @thomaseizinger!

Interesting idea! I'm not against it, and I have a couple of thoughts.

First is about the quality of str0m's ICE implementation. I implemented it by reading the RFC, and it certainly seems to serve the purpose we use it for, which is client-server. There is however a blind spot for peer-peer (see #405). @pthatcher has mentioned that libWebRTC ICE implementation is top of the class, and that str0m may be better off porting that.

Related to that, str0m's ICE isn't strictly RFC compliant – it's compatible. A simplifying assumption is that there is one agent instance per peer connection, and there is only a single stream/component to negotiate. A fully compliant implementation would be able to use one agent for multiple sessions supporting many streams. How much this matters for crate-ifying the code, I don't know, but it should at least be a conscious choice that isolating the code out from str0m does not make a full ICE agent.

We should consider the long term goals of such crate. Full RFC compliance? Line-by-line translation of libWebRTC? Nothing more than we have?

I don't think refactoring it out would be that hard. We could keep it in the str0m git project, and release a separate crate. The majority of the work is likely to be the API documentation, README, examples etc. I don't have the time to do that myself right now, but I also don't think it's right to release something half baked.

A first step could be to land some patches to export some APIs from the ice module under a feature flag, with the intention to make a separate crate once we ironed out the details of this ICE implementation.

@morajabi might have thoughts too.

[thomaseizinger]

Thank you for the comment!

Related to that, str0m's ICE isn't strictly RFC compliant – it's compatible. A simplifying assumption is that there is one agent instance per peer connection, and there is only a single stream/component to negotiate.

This is okay for us. We don't even want any streams, I just want to use ICE to hole-punch a connection!

We should consider the long term goals of such crate. Full RFC compliance? Line-by-line translation of libWebRTC? Nothing more than we have?

I'd be pragmatic here and suggest that to begin with, it can be whatever is useful to you and whichever other projects come along. Full RFC compliance could be a novel goal although I'd favor compatibility over it. Most notably for our usecase, only str0m <> str0m needs to be compatible!

I don't think refactoring it out would be that hard. We could keep it in the str0m git project, and release a separate crate. The majority of the work is likely to be the API documentation, README, examples etc. I don't have the time to do that myself right now, but I also don't think it's right to release something half baked.

A first step could be to land some patches to export some APIs from the ice module under a feature flag, with the intention to make a separate crate once we ironed out the details of this ICE implementation.

I've done this in my fork but hit a roadblock when it came to openssl. I think it would be great if we don't have to extract a crate to start with. To make openssl optional, a fair amount of feature-flagging would be necessary. Would you be open to that?

The problem with openssl is that we are going to deploy this to all sorts of targets (Android, iOS, Windows, Linux, MacOS) and requiring openssl as a dependency even we don't actually use it is something I'd like to avoid.

[morajabi]

We should consider the long term goals of such crate

I agree. Based on @thomaseizinger's comment, the goal would be to feature flag openssl (and maybe like how reqwest allows rustls as an option? not sure if it's similar). Seems like doable within the str0m git repo and with feature flags. @algesten what do you think, is this something generally useful longterm?

[algesten]

Oh, interesting that openssl is the problem!

I've done this in my fork but hit a roadblock when it came to openssl. I think it would be great if we don't have to extract a crate to start with.

I agree. Based on @thomaseizinger's comment, the goal would be to feature flag openssl

Because I favor a pure Rust library, my long term goal for str0m is to use rustls as default and openssl as an optional alternative. However rustls isn't ready for that (yet).

To make openssl optional, a fair amount of feature-flagging would be necessary. Would you be open to that?

Yes and no! Let's do feature flags, but before that, let's refactor the crypto stuff out to an abstraction so we don't get feature flags everywhere. openssl is a bit all over the place because it exposes API for both higher level crypto stuff like DTLS as well as lower level AES primitives. In a pure Rust solution, this would require more libraries – some combo of rustls + ring + sha1 + …

From the top of my head we use openssl in some capacity for this:

• X509 certificate generation for DTLS – long term maybe ring + something?
• DTLS – rustls hopefully will have at some point, and we should help out to fix that
• SRTP encryption – two algorithms right now: AES 128 CM + SHA1 or AEAD AES 128 GCM – ring + sha1 crate
• STUN hmac - sha1 crate (already somewhat abstracted)

I think we should refactor out these into a number of traits, make str0m use Box<dyn Trait> in the relevant places. I think the best would be 4 traits corresponding to the above, but I'm open to suggestions. That way we can openssl implementation to a central place, and feature flag proliferation kept to a minimum.

It's time for mod crypto! :)

[thomaseizinger]

Okay thank you for your input! I'll see what I can come up with :)

[pthatcher]

Sorry for being late to the conversation. A few thoughts:

1. I would prefer 1 str0m crate with feature flags to change what dependencies are needed. Maybe feature flags for RTP, SCTP, DTLS, etc.

2. I think having pluggable crypto is a good idea. People often have inflexible needs around crypto dependencies.

3. Implementing the full RFC for ICE is neither necessary nor desirable. Only a subset is needed. The short version of that is: be compliant with the format of the ICE checks and responses, always send responses, don't send checks too fast (probably not more often than every 5ms), but not too slow (probably every 5ms), it's best to honor the controlling side's nomination (although not strictly necessary). It's better to use only one "component", and you can probably ignore everything involving components, freezing, foundations, and DSCP.

4. I still think a "good" ICE implementation should exist outside of libwebrtc (one that works well with mobile clients that can change networks frequently), and str0m might one day be that. But for a lot of people, "plain" ICE (without continual nomination) is probably good enough.

[thomaseizinger]

Just as a heads-up, I've now started work on our connectivity layer that is based on str0m. Tracking issue here: firezone/firezone#2966. PRs incoming :)

[algesten]

Cool project!

[thomaseizinger]

After #446 is merged, I would start on making openssl pluggable. What do people think about the following:

• Introduce on-by-default openssl feature
• Introduce a Dtls trait that covers the entire API from the Dtls struct
• Make the dtls field in Rtc a Box<dyn Dtls>
• Feature-flag the current Rtc::new constructor to require openssl

This would be minimal disruption for existing users. Some alternatives (not necessarily mutually exclusive with each other):

• introduce a dtls parameter on Rtc::new
• Not enable openssl by default
• Introduce multiple constructors: Rtc::with_openssl

Thoughts?

[algesten]

• Introduce on-by-default openssl feature

Sounds good!

• Introduce a Dtls trait that covers the entire API from the Dtls struct

Yep.

• Make the dtls field in Rtc a Box<dyn Dtls>

Generally in str0m we've favored static dispatch. It probably doesn't make a huge difference,
but let's keep doing that to keep it consistent.

See for instance how we handle packetizer where a containing enum implements the same type as the enum variants: https://github.com/lolgesten/str0m/blob/main/src/packet/mod.rs#L239

• Feature-flag the current Rtc::new constructor to require openssl

I think feature flags should enable config options, rather than change behavior. I.e. we will have the feature openssl on by default and Rtc::new() behaves like today. When we add another DTLS impl (and feature flag it), Rtc::new() doesn't change behavior, it still instantiates openssl, but there is a config option RtcConfig::use_dtls(DtlsImpl), or some such.

The reason for this is that feature flags are additive, and although it's unlikely str0m will ever be used in a way that pulls it in with different feature flags enabled, I think it's least surprising if behavior stays the same regardless.

If the user disables openssl, then we should probably remove Rtc::new as well.

Sound ok?

[thomaseizinger]

• Feature-flag the current Rtc::new constructor to require openssl

I think feature flags should enable config options, rather than change behavior. I.e. we will have the feature openssl on by default and Rtc::new() behaves like today. When we add another DTLS impl (and feature flag it), Rtc::new() doesn't change behavior, it still instantiates openssl, but there is a config option RtcConfig::use_dtls(DtlsImpl), or some such.

The reason for this is that feature flags are additive, and although it's unlikely str0m will ever be used in a way that pulls it in with different feature flags enabled, I think it's least surprising if behavior stays the same regardless.

If the user disables openssl, then we should probably remove Rtc::new as well.

Fully agreed! I didn't mean to suggest that feature flags should change behviour.

It is just that in the absence of another DTLS impl, having a ctor like Rtc::with_openssl is kind of odd. Hence, my suggestion is to kick that can down the road and decide on how to make DTLS configurable when a 2nd impl is being added. Until then, Rtc::new would only be available if the openssl feature is activated. With it being enabled by default, nothing changes for existing users.

Say a rustls-backed implementation comes along, we could then remove Rtc::new and add two new ctors. Or we could add a parameter to new. Either way will be a breaking change for users, signaling them that they have to make a decision now which backend to use.

[thomaseizinger]

I've submitted a draft here: #447, please have a look.

[thomaseizinger]

This is resolved, thank you!