Repository navigation
feat(calls): end-to-end encrypted calls - #6793
AndyScherzinger wants to merge 3 commits into
Conversation
📱 QA build
The QA build installs alongside a released Nextcloud app, so you can keep Downloading the file requires a GitHub account, so open this link on the |
507145d to
dda32f3
Compare
11ea751 to
d7d6bdb
Compare
Port of the web client's e2ee/encryption.js (and the iOS port of it): every participant generates a random frame key and sends it to each other session through a pairwise Olm session over signaling messages, ratchets it when someone joins and rotates it when someone leaves. The Olm account and the frame key rings are behind two small interfaces, so the protocol runs and is tested on the JVM; the vodozemac and WebRTC backed implementations come with their AARs. Signaling routes "message" payloads of the "encryption.*" types to a new listener, the payload model gains the key exchange fields, and the capability check now requires both the signaling feature and the server config. Assisted-by: Claude Code:claude-fable-5-1 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Calls announce the "encryption" feature to the signaling server, the key exchange lives for one joined room and signaling session, and the peer connections attach an encryptor to every sender of the publisher and a decryptor to every receiver of a subscriber, preferring VP8 as the only codec the frame encryption handles. When the server requires encrypted calls but there is no high-performance backend with MCU, or the key exchange could not be set up, the call ends instead of sending media unencrypted. The old "calls not supported" checks and notification are removed. The factory creating the key exchange still reports encryption as unavailable until the WebRTC build with the frame encryption and the vodozemac bindings are published. Assisted-by: Claude Code:claude-fable-5-1 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The guard clauses mirror the web client's code; detekt's ReturnCount would otherwise push the project over its issue threshold. Assisted-by: Claude Code:claude-fable-5-1 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
d7d6bdb to
5c4fa8f
Compare
📝 WalkthroughWalkthroughThe changes add encryption signaling models, Olm and frame-crypto interfaces, and a per-session key exchange. WebSocket signaling conditionally advertises encryption and manages exchange lifecycle. CallActivity connects available key rings to peer connections, which attach frame cryptors and select VP8 when key rings are present. Call-start and incoming-call paths no longer block calls based on the earlier unsupported-encryption check. The factory currently marks frame encryption unavailable. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Encrypted calls can, in a narrow race, set up media connections without encryption instead of ending the call. Native key resources also accumulate as participants leave. Close the fail-open path before merging; the other issues are small fixes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 12.66% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 158 functions across 21 files. (1 skipped: 1 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
063971e9-7fa2-48d7-b3da-fdac260b302d
📒 Files selected for processing (25)
app/src/main/java/com/nextcloud/talk/activities/CallActivity.ktapp/src/main/java/com/nextcloud/talk/call/e2ee/CallEncryption.ktapp/src/main/java/com/nextcloud/talk/call/e2ee/CallEncryptionFactory.ktapp/src/main/java/com/nextcloud/talk/call/e2ee/EncryptionMessage.ktapp/src/main/java/com/nextcloud/talk/call/e2ee/FrameCrypto.ktapp/src/main/java/com/nextcloud/talk/call/e2ee/OlmCrypto.ktapp/src/main/java/com/nextcloud/talk/callnotification/CallNotificationActivity.ktapp/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.ktapp/src/main/java/com/nextcloud/talk/jobs/NotificationWorker.ktapp/src/main/java/com/nextcloud/talk/models/json/signaling/NCMessagePayloadDto.ktapp/src/main/java/com/nextcloud/talk/models/json/signaling/OlmMessageDto.ktapp/src/main/java/com/nextcloud/talk/signaling/EncryptionMessageNotifier.ktapp/src/main/java/com/nextcloud/talk/signaling/SignalingMessageReceiver.ktapp/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.ktapp/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.javaapp/src/main/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelper.javaapp/src/main/java/com/nextcloud/talk/webrtc/WebSocketInstance.ktapp/src/main/res/values/strings.xmlapp/src/test/java/com/nextcloud/talk/call/e2ee/CallEncryptionTest.ktapp/src/test/java/com/nextcloud/talk/models/json/signaling/NCMessagePayloadDtoEncryptionTest.ktapp/src/test/java/com/nextcloud/talk/signaling/SignalingMessageReceiverEncryptionTest.ktapp/src/test/java/com/nextcloud/talk/utils/CapabilitiesUtilCallEncryptionTest.ktapp/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperEncryptionTest.ktapp/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperTest.kt
💤 Files with no reviewable changes (3)
- app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt
- app/src/main/java/com/nextcloud/talk/callnotification/CallNotificationActivity.kt
- app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| hasMCU: Boolean, | ||
| sessionId: String? | ||
| ): Pair<FrameKeyRing?, FrameKeyRing?> { | ||
| val callEncryption = callEncryption() ?: return null to null |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Fail closed in keyRingsFor when encryption is required.
When isCallEndToEndEncryptionEnabled is true and callEncryption() returns null, keyRingsFor returns null to null. PeerConnectionWrapper then attaches no frame encryptor and prefers H264. The publisher guard ensureCallEncryption() in handleCallParticipantsChanged runs on a different thread than this creation code.
WebSocketInstance.closeCallEncryption() sets callEncryption to null whenever the hello response carries a new session or the client leaves the room. If that happens between the guard and the wrapper creation, the MCU publisher is created without an encryptor and sends plaintext frames. Subscriber wrappers created from offerMessageListener also skip the guard.
Treat a missing key exchange as a fatal error here, not as an unencrypted call.
🔒️ Proposed fix
- val callEncryption = callEncryption() ?: return null to null
+ if (!isCallEndToEndEncryptionEnabled) {
+ return null to null
+ }
+ val callEncryption = callEncryption()
+ ?: throw IllegalStateException("Call has to be end-to-end encrypted but has no key exchange")Throwing is one option. Alternatively, return a nullable result and have getOrCreatePeerConnectionWrapperForSessionIdAndType call failCallEncryption(R.string.nc_call_e2ee_setup_failed) without creating the wrapper.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val callEncryption = callEncryption() ?: return null to null | |
| if (!isCallEndToEndEncryptionEnabled) { | |
| return null to null | |
| } | |
| val callEncryption = callEncryption() | |
| ?: throw IllegalStateException("Call has to be end-to-end encrypted but has no key exchange") |
| synchronized(remoteKeyRings) { | ||
| for (sessionId in sessionIds) { | ||
| sessions.remove(sessionId) | ||
| remoteKeyRings.remove(sessionId) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Dispose the key rings of sessions that leave.
usersLeft removes each FrameKeyRing from remoteKeyRings but never calls dispose(). Only close() disposes key rings. Each participant that leaves therefore leaks one key ring for the rest of the call. In production that key ring is backed by native WebRTC memory. The FrameKeyRing contract says encryptors and decryptors keep working after dispose(), so disposing here is safe for decryptors that are still attached.
♻️ Proposed fix
--- "a/app/src/main/java/com/nextcloud/talk/call/e2ee/CallEncryption.kt"
+++ "b/app/src/main/java/com/nextcloud/talk/call/e2ee/CallEncryption.kt"
@@ -101,12 +101,12 @@
scope.launch {
if (isClosed) return@launch
synchronized(remoteKeyRings) {
for (sessionId in sessionIds) {
sessions.remove(sessionId)
- remoteKeyRings.remove(sessionId)
+ remoteKeyRings.remove(sessionId)?.dispose()
}
}
// A new key keeps participants that left from decrypting what is sent from now on
rotateJob = debounce(rotateJob) { rotateKey() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| synchronized(remoteKeyRings) { | |
| for (sessionId in sessionIds) { | |
| sessions.remove(sessionId) | |
| remoteKeyRings.remove(sessionId) | |
| } | |
| } | |
| synchronized(remoteKeyRings) { | |
| for (sessionId in sessionIds) { | |
| sessions.remove(sessionId) | |
| remoteKeyRings.remove(sessionId)?.dispose() | |
| } | |
| } |
|
|
||
| @Parcelize | ||
| @JsonObject | ||
| @TypeParceler<Any?, AnyParceler> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C3 'AnyParceler' --type=kotlin --type=java
rg -n -C3 'NCSignalingMessageDto|NCMessagePayloadDto' --type=kotlin --type=java | rg -n 'putParcelable|putExtra|writeToParcel|Bundle'Repository: nextcloud/talk-android
Length of output: 10435
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- AnyParceler.kt ---'
nl -ba app/src/main/java/com/nextcloud/talk/models/json/AnyParceler.kt
printf '%s\n' '--- NCMessagePayloadDto.kt ---'
nl -ba app/src/main/java/com/nextcloud/talk/models/json/signaling/NCMessagePayloadDto.kt
printf '%s\n' '--- NCSignalingMessageDto references ---'
rg -n -C4 -F -- 'NCSignalingMessageDto' app/src/main || true
printf '%s\n' '--- NCMessagePayloadDto parceling references ---'
rg -n -C4 -F -- 'NCMessagePayloadDto' app/src/main || true
printf '%s\n' '--- Parcelable transport calls for signaling DTOs ---'
rg -n -C3 'putParcelable|putParcelableArrayList|putExtra|writeToParcel|Bundle' app/src/main --glob '*.kt' --glob '*.java' | rg -n 'signaling|Signaling|Payload|Dto|Bundle|putParcelable|putExtra|writeToParcel' || true
printf '%s\n' '--- base-to-head diff for affected files ---'
git diff --no-ext-diff --unified=20 91ad98153998387c41ed8994e97da7a9cf2c7f0b 5c4fa8fab233345cc924a5399f1aa95b6002ff45 -- app/src/main/java/com/nextcloud/talk/models/json/AnyParceler.kt app/src/main/java/com/nextcloud/talk/models/json/signaling/NCMessagePayloadDto.kt app/src/main/java/com/nextcloud/talk/models/json/signaling/NCSignalingMessageDto.ktRepository: nextcloud/talk-android
Length of output: 41628
Pass the value to Parcel.writeValue.
NCMessagePayloadDto maps key: Any? to AnyParceler. AnyParceler.write ignores the value and passes the Parcel object to writeValue, which can fail when a caller parcels the DTO. The repository does not establish a current Bundle or Intent path for these signaling DTOs, so avoid claiming that an existing app workflow always crashes.
🐛 Suggested fix in AnyParceler.kt
override fun Any?.write(parcel: Parcel, flags: Int) {
- parcel.writeValue(parcel)
+ parcel.writeValue(this)
}1 participant
Android side of end-to-end encrypted calls, compatible with the web client (nextcloud/spreed#14005, nextcloud/spreed#14098) and the iOS client (nextcloud/talk-ios#2735). Port of the web client's
e2ee/encryption.js: every participant sends its own frame key to each other session through a pairwise Olm session over signaling messages, ratchets it when someone joins and rotates it when someone leaves. Like web and iOS this only works through the high-performance backend with MCU, forces VP8, and ends the call instead of sending media unencrypted when encryption cannot be set up.The native pieces come from two other PRs and are not published yet, so
CallEncryptionFactorystill reports encryption as unavailable. Until then an E2EE-enabled server gets "could not be set up" and the call ends; nothing is sent in plain text.🖼️ Screenshots
No UI changes beyond two new toasts; the old "calling is not supported because end-to-end encryption is enabled" notice and notification are gone.
🚧 TODO
TalkKeyRing→FrameCryptoandVodozemacAccount→OlmCryptoinCallEncryptionFactory,IS_AVAILABLE = true, dependencies inbuild.gradle.kts🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)