-
Notifications
You must be signed in to change notification settings - Fork 106
feature(e2ee): add data channel encryption #708
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
base: main
Are you sure you want to change the base?
Conversation
)); | ||
} | ||
Err(e) => { | ||
log::warn!("Failed to encrypt data packet: {}", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If E2EE is enabled and encryption fails, it seems to me publish_data
should return the error and not continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To do that, I think the best approach is to define a new error type (i.e. EncryptionError
) and add a case for it in the RoomError
enum. I'm not sure how much information can be extracted from the underlying exception from libwebrtc, so it probably makes sense to just dump the error string inside EncryptionError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't introducing a new error type be a breaking change for match clauses? 🤔
there's no non_exhaustive
declared for it afaict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, I had assumed it was declared as non exhaustive—it should be for v3. Given that it isn't, I think using the existing Rtc
case is the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for Rtc
I'm not able to supply a string argument and the error type is defined as webrtc internal error
.
I was going for Internal
as an error in order to be able to supply a reason
}; | ||
|
||
#[tokio::main] | ||
async fn main() -> Result<(), Box<dyn Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice example!
pub auto_subscribe: bool, | ||
pub adaptive_stream: bool, | ||
pub dynacast: bool, | ||
#[deprecated(note = "Use `encryption` field instead, see x for a detailed explanation")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have something ready to link for "x"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently wip, will wait for the actual link to update and merge
this PR adds data channel encryption capabilities.
For backwards compatibility this is not enabled on existing implementations.
Instead
RoomOptions.e2ee
is being deprecated (no dc encryption) and a newRoomOptions.encryption
field is introduced which enables data channel encryption