feat: v8 encryption modes

[tignear]

close #246

• crypto_mode as a parameter for negotiation
• test aes256gcm voice encryption
• test xchacha20poly1305 voice encryption
• test aes256gcm voice decryption
• test xchacha20poly1305 voice decryption
• test the behavior when a silence frame is input

[FelixMcFelix]

Hi, thanks for putting this together. I am actively working on this in a local branch. I'm hoping to finish up the receive side today -- I'm not sure where our implementations differ so it'll take me some time to reconcile that.

[tignear]

The decryption was successful, but I have not confirmed whether it is a correct opus packet.

[FelixMcFelix]

The most notable change I have between us is that my PR reuses crypto_mode as a parameter for negotiation (i.e., it's a preference rather than an explicit choice). There is a reason I've done this, which is that the discord docs seem to suggest that not all nodes implement Aes256Gcm -- this may no longer be the case, but I like to err on the safe side. I think there are some semver breakages here in your implementation, but those are mainly my fault for making these methods too public.

If you don't mind, I'd be happy to try and fuse the two branches together via your PR? Github should recognise this by correctly listing co-authorship. Let me know if you're happy with me doing so.

[tignear]

Of course, that's fine, but my implementation seems to have some issues (especially the Mixer resource leak, which seems particularly nasty).

[FelixMcFelix]

I'm currently working on merging my changes in (negotiation, make sure we don't break semver). If you could please hold off for a while so I don't have a moving target that would be grand.

[FelixMcFelix]

Hey, thanks for being patient. A couple of notes explaining why some things have moved around during integration:

• A bunch of your methods returned the encryption tag as a Vec<u8> -- causing a mandatory per-packet memory allocation like this is less than ideal. I've restructured to avoid this.
• There is no need to make CryptoState variants deprecated -- it's internal-only.
• MixType::Passthrough should refer only to payload bytes -- the prefix and suffix of the encryption mode do not factor into the bytes we count here.
• A bunch of methods are, sadly, exposed publicly. There aren't any changes in their signatures allowed, so there are some awful methods needed to workaround them. The ones I caught are below:
  • CryptoMode::payload_prefix_len -> CryptoMode::payload_prefix_len2 (ugh)
  • CryptoMode::encrypt_in_place -> [deprecated].

I've put a bunch more work & research into receive -- I can verify that the new modes send/receive correctly on [Normal, Aes256Gcm, XChaCha20Poly1305]. I've tested both mixed and passthrough audio -- for reference, silent frames are sent after any source ends, so those are tested too.

I'll give this another read, let me know if you have any questions/objections about the changes?

[tignear]

I think it's good. I misunderstood that crypto_secretbox can't use AEAD. Sorry.

[tignear]

I have some concerns about how crypto_mode is retrieved from the config, so I'll test it and let you know.

[tignear]

When Normal is set in config and Aes256Gcm is selected by negotiation, the audio will be noticeably distorted because the CryptoMode used by Mixer is different from the actual CryptoMode. However, if XChaCha20Poly1305 is selected when trying to use Aes256Gcm, the packet structure is the same, so there is no problem.

[FelixMcFelix]

Very good catch, I've given all the modes a quick check again and seemingly no issues. Thanks for all the work here. Give me a ping when you're happy to merge, and we can get a patch release out prior to the salsa20 switchoff.

[tignear]

@FelixMcFelix
The merge looks good to me. Thank you for all your hard work.