Support all voice encryption modes #599
Conversation
There was a problem hiding this comment.
This is really solid work. I needed quite some time to read into the papers and understand it, but this code is really easy to digest (probably in large part thanks to binary matching).
I have a bunch of smaller questions that I posted as part of the review. I also have two other questions:
- Do you think it is worth it to add some basic unit tests for the custom crypto?
- Do you think we should contribute this to OTP?
lib/nostrum/voice/crypto.ex
Outdated
|
|
||
| @type cipher :: cipher_non_rtpsize() | cipher_alias() | cipher_rtpsize() | ||
|
|
||
| defp mode, do: Application.get_env(:nostrum, :voice_encryption_mode, :aes256_gcm) |
There was a problem hiding this comment.
Do you reckon there is a need to switch this at runtime? Or should we make this a compile-time setting?
There was a problem hiding this comment.
I originally had this a compile time config but changed it to runtime for my encryption mode test suite so I didn’t have to recompile each time. I can change it back
There was a problem hiding this comment.
The test suite in question
def voice_encryption_test_suite do
[
:xsalsa20_poly1305,
:xsalsa20_poly1305_suffix,
:xsalsa20_poly1305_lite,
:xsalsa20_poly1305_lite_rtpsize,
:xchacha20_poly1305,
:aead_xchacha20_poly1305_rtpsize,
:aes256_gcm,
:aead_aes256_gcm,
:aead_aes256_gcm_rtpsize
]
|> Enum.each(&voice_encryption_test/1)
end
def voice_encryption_test(mode) do
IO.puts("Starting test with #{mode}")
Application.put_env(:nostrum, :voice_encryption_mode, mode)
Dummy.join_voice()
Dummy.sleep_until_ready()
Dummy.play_file()
Process.sleep(3_000)
Dummy.stop()
IO.puts("Speak now!")
Dummy.playback_raw(80)
Process.sleep(1_500)
Dummy.leave_voice()
IO.puts("Test complete")
Process.sleep(1_500)
end
…tpsize voice encryption modes
…de config compile time.
lib/nostrum/voice/crypto.ex
Outdated
| defp mode, do: Application.get_env(:nostrum, :voice_encryption_mode, :aes256_gcm) | ||
| @mode Application.compile_env(:nostrum, :voice_encryption_mode, :aes256_gcm) | ||
|
|
||
| def encryption_mode do | ||
| mode = mode() | ||
|
|
||
| Map.get( | ||
| %{ | ||
| xchacha20_poly1305: "aead_xchacha20_poly1305_rtpsize", | ||
| aes256_gcm: "aead_aes256_gcm_rtpsize" | ||
| }, | ||
| mode, | ||
| "#{mode}" | ||
| @mode, | ||
| "#{@mode}" | ||
| ) | ||
| end |
There was a problem hiding this comment.
Yak shaving here but since this is compile time config now we might be able to remove the dynamic apply and Map.get to figure out the proper name and just resolve the function to encrypt and decrypt at compile time - what do you think?
|
I've added some unit tests and internal docs |
|
Thanks! |
No description provided.