signalapp / libsignal

Home to the Signal Protocol as well as other cryptographic primitives which make Signal possible.
GNU Affero General Public License v3.0
3.08k stars 362 forks source link

remove unnecessary Result-wrapping in `crypto.rs` #474

Closed cosmicexplorer closed 1 year ago

cosmicexplorer commented 2 years ago

Broken out from #287.

  1. Separate CTR and CBC encryption methods in ctr and cbc submodules.
  2. Define pub const sizes for array lengths.
  3. Require fixed-size arrays to en/decryption methods to remove EncryptionError and one case of DecryptionError.
  4. Rename methods to aes_256_* and aes_256_ctr_hmac_* for consistency.

TODO

jrose-signal commented 2 years ago

These aren't pub for a reason: if someone wants to use these operations, they should get them directly from the RustCrypto crates. These are pretty much just convenience wrappers because of how they're used within the protocol. (The hardcoded "first ten bytes as a MAC" is something that a generalized library probably wouldn't hardcode.)

I think you're right about EncryptionError, though. Someone has to check that these keys are the right length, but it can happen a level up. And then I think DecryptionError collapses down to a struct instead of a one-case enum (not that it matters so much for an implementation detail).

cosmicexplorer commented 1 year ago

These are pretty much just convenience wrappers because of how they're used within the protocol.

Ok, so in light of this I've changed it to pub(crate) mod crypto; in lib.rs, removed the doctests, and re-focused this PR on just removing unnecessary Results by enforcing fixed-size slices where possible.

However, on top of that, I've also kept in:

I can revert any of that as desired.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This issue has been closed due to inactivity.