-
Notifications
You must be signed in to change notification settings - Fork 164
Encrypted data channel #781
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
Conversation
e25ed0f to
a8ac611
Compare
| _state.mutate { $0.connectOptions = connectOptions } | ||
| } | ||
|
|
||
| await cleanUp() |
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.
Moved due to E2EEManager.cleanUp called after setup
|
@lukasIO would be nice to perform some x-platform test (proto compatibility) |
|
@lukasIO as for x-platform test:
cc @cloudwebrtc maybe it's worth to do another x-check with flutter |
does audio/video keep working when ratcheting on the native client ? |
nope, keys get out of sync. |
|
@lukasIO if you wanna dig deeper, I added one option to the example (2.8.0). Seems like the frame logic may be broken, unrelated to this PR (I haven't tested
BTW, macOS chat is still using old infra, but you should be able to get there with frame encryption. I can help you real-time if needed. For me:
so looks like the issue is on JS side (custom logic vs rtc)? |
|
Looks like the above behavior is by design, for the shared key it should still be: |
This reverts commit d569de4.
|
Ended up reverting that [extended ratchet window] ⬆️ |
| delegates.notify { | ||
| $0.dataChannel(self, didReceiveDataPacket: dataPacket) | ||
| if let encryptedPacket = dataPacket.encryptedPacketOrNil, | ||
| let e2eeManager, e2eeManager.isDataChannelEncryptionEnabled |
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.
decrypting should also work when data channel encryption is not explicitly enabled.
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.
thought about that, yep it makes more 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.
It does not apply to situations where e2ee is explicitly disabled (no options passed).
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!
lukasIO
left a comment
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.
from the encryption workflow perspective this looks good to me!
|
|
||
| /// Failed to decrypt a data packet when encryption is enabled. | ||
| @objc optional | ||
| func room(_ room: Room, didFailToDecryptDataWithEror error: LiveKitError) |
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.
Maybe it would be good to pull participant id here?
|
@hiroshihorie feel free to take a look at the Swift side, I don't see any obvious improvements atm. |
|
|
||
| var isOpen: Bool { _state.isOpen } | ||
|
|
||
| var e2eeManager: E2EEManager? |
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.
Maybe this can be weak ? I think the Room holds the instance. 🤔
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.
I'd be careful with that - I mean, there's no point in making sure that there's only 1 strong ref... if there's no cycle here... Let's double-check if the DC is deallocated properly (if needed).
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.
We gotta move'm to submodules finally, it's hard to track these subtle changes.
|
Merging as macOS build must be fixed on |


currentKeystate toBaseKeyProvider0sharedKeyby default 🎉Migration/deprecations
E2EEOptionswithEncryptionOptions(1:1)EncryptionTypeparameter toRoom/ParticipantDelegatemethods