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 aes256_gcm and retire deprecated methods #10066

Closed
wants to merge 1 commit into from

Conversation

Aviana
Copy link

@Aviana Aviana commented Jan 3, 2025

  • Implement aes256_gcm_rtpsize
  • Implement automatic fallback to xchacha20_poly1305_rtpsize on unsupported hardware
  • Switch from PyNaCl to libnacl

Summary

While i am aware of the refusal of PyNaCl to implement AES256-GCM it is still the recommended method by discord themselves. Also the points brought up against it seem to not apply to this implementation. 350GB data on a single connection is ridiculous and there is automatic fallback for unsupported hardware.

Related issues:
#10012

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • 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, ...)

- Implement aes256_gcm_rtpsize
- Implement automatic fallback to xchacha20_poly1305_rtpsize on unsupported hardware
- Switch from PyNaCl to libnacl
@mikeshardmind
Copy link
Contributor

mikeshardmind commented Jan 4, 2025

libnacl is not actively maintained, and there's really not a good reason to use it over pynacl.

The related issue is already covered by another change: #9953

There are other reasons not to use AES256_GCM, some of which involve the fact that libsodium (which both pynacl an libnacl are both wrapping) requires specific CPU instructions to be available, which is information you yourself linked to as it was part of the refusal:

libsodium's implementation is limited to x86 CPUs implementing SSE3, aesni and pclmul

This would limit the devices discord.py works on in a way that the other accepted change does not. (edit: fallback is fine)

Another one of the other reasons is that AES256_GCM uses more resources without actually being a better choice here cryptographically.

@ferrenza
Copy link

ferrenza commented Jan 4, 2025

Excuse me for joining in, I don't know much about this. On one hand, AES256-GCM is indeed recommended by Discord itself. On the other hand, libnacl is quite outdated as it was last developed 2 years ago . But does PyNaCl fully support AES256-GCM? If so, why isn't it maintained since it's already proven to be stable?

@Aviana
Copy link
Author

Aviana commented Jan 4, 2025

libnacl is not actively maintained, and there's really not a good reason to use it over pynacl.

The last release of PyNaCl in pypi is exactly 3 years ago while the last release of libnacl was around 1.5 years ago according to their respective github / pypi.org pages. This could be from irrelevant (a linux system where the systems libsodium is used) to worse when you get it off of pypi.org with an outdated libsodium.

The related issue is already covered by another change: #9953

I know this and not ticking the "this fixes an issue" box is an indicator of that. However the issue is still open and therefore relevant.

Another one of the other reasons is that AES256_GCM uses more resources without actually being a better choice here cryptographically.

There are implementations of AES256-GCM that do not require certain features in hardware it is those that are generally considered slower. However this is not relevant to this pull request as it falls back to XChaCha20 in that case.

I have read through several blogposts / articles regarding this and the general consensus seems to be that there is no clear winner.
However if i have to make a guess why discord is recommending AES256_GCM it might be because it has been in the wild for far longer and is used in critical government infrastructure. And in my research i have not found a single other voice client implementation that did not switch to AES256_GCM as their default after the shutdown of the old methods in the end of November.

In the end i don't really care as i see no negatives to either accept or deny this pull request. It is just meant to follow upstream (discord) best practice.

@mikeshardmind
Copy link
Contributor

Discord's recommended choice is not something I would choose to tie the phrase "best practice" to. You're talking about the company that decided sending a bitfield as an array rather than an integer was at some point a good idea.

The warning from libsodium, written by actual cryptographers covers multiple reasons to prefer something else for AEAD

libnacl is not actively maintained, and there's really not a good reason to use it over pynacl.

The last release of PyNaCl in pypi is exactly 3 years ago while the last release of libnacl was around 1.5 years ago according to their respective github / pypi.org pages. This could be from irrelevant (a linux system where the systems libsodium is used) to worse when you get it off of pypi.org with an outdated libsodium.

I actually wasn't referring to last commit. I'm aware that software can be stable and require no further changes. libnacl does not work with up to date versions of libsodium: saltstack/libnacl#135 and this was reported in 2023

In that issue:

Assigned myself and then noticed not a salt repo issue, so removed myself.
Salt has stopped needing libsodium as a dependency a few releases back, but will make use of it if present.
This issue is more for the libnacl repository, which with Salt 3006.2 now using PyNaCl, doesn't matter as much to Salt anymore.

@Aviana Aviana closed this Jan 4, 2025
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.

3 participants