Skip to content
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

Implement AES Encryption for Voice #897

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

viztea
Copy link
Contributor

@viztea viztea commented Dec 12, 2023

I tested voice send/receive for all 3 xsalsa20 poly1305 nonce strategies, all work as expected.

Copy link
Member

@DRSchlaubi DRSchlaubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please run ./gradlew apiDump

@lukellmann
Copy link
Member

i'm not very familiar with voice connections, can you explain a bit why this is needed?

@viztea
Copy link
Contributor Author

viztea commented Dec 20, 2023

i'm not very familiar with voice connections, can you explain a bit why this is needed?

It kinda is and kinda isn't. I initially wanted to implement AES because it was faster than xsalsa but after doing a couple benchmarks it looks like xsalsa is a little bit more consistent and a couple microseconds faster.

This isn't really that surprising considering Kord Voice isn't the most efficient voice library, it doesn't help that Java relies on ByteBuffers to be performant.

Comment on lines 170 to 174
val encryption = if ((receiveVoice || streams != null) && encryption?.supportsDecryption != true) {
VoiceEncryption.XSalsaPoly1305()
} else {
encryption ?: VoiceEncryption.AeadAes256Gcm
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to log if the user's chosen encryption method ends up not being used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or even throw an exception? but logging a warning is probably best.

@lost-illusi0n
Copy link
Contributor

Functionality-wise LGTM. It likely will make adding/working with encryption a bit easier in the future too, if (or rather when) Discord decides to change things up.

Comment on lines 170 to 174
val encryption = if ((receiveVoice || streams != null) && encryption?.supportsDecryption != true) {
VoiceEncryption.XSalsaPoly1305()
} else {
encryption ?: VoiceEncryption.AeadAes256Gcm
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or even throw an exception? but logging a warning is probably best.

voice/src/main/kotlin/encryption/VoiceEncryption.kt Outdated Show resolved Hide resolved
voice/src/main/kotlin/encryption/VoiceEncryption.kt Outdated Show resolved Hide resolved
@@ -27,4 +23,17 @@ public sealed interface NonceStrategy {
* Writes the [nonce] to [cursor] in the correct relative position.
*/
public fun append(nonce: ByteArrayView, cursor: MutableByteArrayCursor)

public interface Factory {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this factory? i'd rather not add it if not needed.

voice/src/main/kotlin/encryption/VoiceEncryption.kt Outdated Show resolved Hide resolved
voice/src/main/kotlin/encryption/VoiceEncryption.kt Outdated Show resolved Hide resolved
@viztea
Copy link
Contributor Author

viztea commented Jun 5, 2024

  • Implemented voice receive for AES, had to refactor a bit of the code to support passing of AEAD data.
  • Removed VoiceEncryption#supportsDecryption
  • Removed the sealed modifier for VoiceEncryption to allow external implementations (??)
  • Added kdoc to the VoiceEncryption interface.

Not sure if removing supportsDecryption is a good idea with it being an open interface now 🤷🏼

@viztea viztea marked this pull request as draft June 9, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants