Add e2ee for data channel messages #1595
Conversation
🦋 Changeset detectedLatest commit: 57037da The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Quick question @lukasIO, is it necessary to do a key ratchet attempt after data decrypt fails? |
|
oh, that's a good point. I'll look at adding that! |
src/room/Room.ts
Outdated
| const dcEncryptionEnabled = false; | ||
| const e2eeOptions = this.options.e2ee; | ||
|
|
||
| // TODO(dc-e2ee): add this back in |
There was a problem hiding this comment.
is this important in this PR?
There was a problem hiding this comment.
this needs to be uncommented once we expose the options.encryption field.
Because this PR doesn't actually declare that, we leave the logic commented out
There was a problem hiding this comment.
@lukasIO are you okay with creating the "new" options type right now and making it internal? I need that mostly for integration tests (to drive the feature), but maybe that's more elegant than the opt-in flag dcEnabled.
There was a problem hiding this comment.
for testing or in general?
I've been thinking more about the "general" route and I think we should go forward with exposing a new options field now and deprecating the old one. v3 will remove the old one then.
There was a problem hiding this comment.
Yup, deprecating now makes much more sense, as e.g. you can deprecate old events/delegates as well, without changing the signatures "silently" so people no longer get their events (enriched with EncryptionType)
| export function asEncryptablePacket(packet: DataPacket): EncryptedPacketPayload | undefined { | ||
| if ( | ||
| packet.value?.case !== 'sipDtmf' && | ||
| packet.value?.case !== 'metrics' && |
There was a problem hiding this comment.
Looks like metrics is encryptable?
livekit/protocol@72e862b#diff-65ff73125c901a7593f1b81ac2dfa070f5d617c4fa3aebb570d36618526c3d89R313
There was a problem hiding this comment.
In theory yes, the question is how that would handled for the dashboard were you'd want to see the metrics
size-limit report 📦
|
|
|
||
| private dataChannelEncryptionEnabled: boolean; | ||
|
|
||
| constructor(options: E2EEManagerOptions, dcEncryptionEnabled: boolean) { |
There was a problem hiding this comment.
Do we really need to expose dcEncryptionEnabled here, or just assign based on the type of options (old vs new)?
There was a problem hiding this comment.
in JS E2EEManager is an internal class, so this is not really a big concern to me.
We can of course also make it part of the E2EEManagerOptions
There was a problem hiding this comment.
ah, I see why I didn't do this in the first place, as the E2EEManagerOptions are actually exposed to users under an alias right now.
- Adds data channel encryption based on livekit/client-sdk-js#1595 - Adds `currentKey` state to `BaseKeyProvider` - It was missing comparing to other platforms, and we cannot explicitly default to `0` - Adds some integration tests, as most of the _crypto_ stuff is tested by webrtc - Existing hi-level tests e.g. data streams will get a `sharedKey` by default 🎉 ### Migration/deprecations - Replace `E2EEOptions` with `EncryptionOptions` (1:1) - Add `EncryptionType` parameter to `Room/ParticipantDelegate` methods --------- Co-authored-by: Hiroshi Horie <[email protected]>
depends on livekit/protocol#1127
Most importantly for a frictionless adoption this PR ships decryption support.
This means if at a later stage encryption will be turned on by default, all clients that have the decryption support from this PR will continue to work.