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

feat(voice): add aead_xchacha20_poly1305_rtpsize encryption mode, remove old modes #1228

Merged
merged 9 commits into from
Nov 13, 2024

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented Aug 13, 2024

Summary

discord/discord-api-docs@fe89a9c#diff-a1da547b9f7beb426699bee30117616d1a5bae58b74cd0751a68909db97ef69bR211-R219

Adds aead_xchacha20_poly1305_rtpsize voice encryption mode (pynacl docs), and removes the previous xsalsa20_poly1305* modes.

Note that this does not implement the other new (and "preferred") aead_aes256_gcm_rtpsize mode, since it's not exposed by PyNaCl despite being part of libsodium.
I don't really want to add another dependency just for this, and it can always be implemented separately by subclassing VoiceProtocol if desired. Alternatively, we could bundle libsodium dlls, similar to libopus, and do all the ctypes interop manually. Also not ideal, though.

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

no call to `checked_add` because once function call is already expensive enough,
and it doesn't get much more simple than that anyway
@shiftinv shiftinv added t: enhancement New feature t: api support Support of Discord API features labels Aug 13, 2024
@shiftinv shiftinv added this to the disnake v2.10 milestone Aug 13, 2024
`x.ljust(24, b"\0")` is surprisingly fast, it's more performant than `x + bytes(20)` and about 3x as fast as the previous `bytearray` construction
@shiftinv
Copy link
Member Author

For possibly future reference, this works for the other new encryption mode, which isn't supported by PyNaCl. Using https://cryptography.io/en/latest/hazmat/primitives/aead/#cryptography.hazmat.primitives.ciphers.aead.AESGCM, others like pycryptodome likely work just fine as well.

def _encrypt_aead_aes256_gcm_rtpsize(self, header: bytes, data) -> bytes:
    from cryptography.hazmat.primitives.ciphers.aead import AESGCM

    box = AESGCM(bytes(self.secret_key))
    # see https://doc.libsodium.org/secret-key_cryptography/aead/aes-256-gcm#notes for nonce length
    nonce, padded_nonce = self._get_nonce(12)

    return header + box.encrypt(padded_nonce, bytes(data), bytes(header)) + nonce

@elenakrittik
Copy link
Contributor

Do you know if there's a specific reason pynacl doesn't expose that part of the API? Maybe it's just an oversight

@shiftinv
Copy link
Member Author

Do you know if there's a specific reason pynacl doesn't expose that part of the API? Maybe it's just an oversight

As far as I can tell based on pyca/pynacl#515 (comment) and a few other comments, this is intentional; pynacl doesn't want to give users a choice.
The aes256-gcm implementation in libsodium relies on AES-NI support in the CPU (which is a reasonable requirement for any CPU released in the last decade, but still); their docs also recommend against using that encryption mode, though in this case that's not our call to make, but Discord's.
cryptography uses openssl bindings instead, which uses AES-NI if available but falls back to a software implementation if not.

It's unfortunate that pynacl doesn't at least supply low-level bindings (without a high-level primitive like nacl.secret.SecretBox/Aead), which would be fine to use here.

the new `aead_xchacha20_poly1305_rtpsize` is supported by all voice servers already.
@shiftinv shiftinv merged commit 4c8e943 into master Nov 13, 2024
29 checks passed
@shiftinv shiftinv deleted the feature/voice-encryption-aead branch November 13, 2024 13:44
shiftinv added a commit that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: api support Support of Discord API features t: enhancement New feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants