Add crypto module #175
Add crypto module #175
Conversation
Update crypto module structure and add enhanced AES-CBC cryptor.
parfeon
added
priority: medium
parfeon
requested review from
mohitpubnub,
kleewho and
marcin-cebo
as code owners
src/lib.rs
Outdated
| //! | `full` | Enables all non-conflicting features | Configuration, Publish, Subscribe, Access Manager, Parse Token, Presence | | ||
| //! | `default` | Enables default features: `publish`, `subscribe`, `serde`, `reqwest`, `aescbc`, `std` | Configuration, Publish, Subscribe | | ||
| //! | `default` | Enables default features: `publish`, `subscribe`, `serde`, `reqwest`, `crypto`, `std` | Configuration, Publish, Subscribe | |
There was a problem hiding this comment.
add crypto into full feature.
btw. do we want to have it as a default one?
There was a problem hiding this comment.
@Xavrax, I will add, however, regarding the issue of default, I do not possess a clear opinion. I would suspect that we enable everything by default, or no?
There was a problem hiding this comment.
I'm not sure if we should enable everything as default.
Imo we should have as small amount default features as possible but with taking to the account dx of our customers.
| getrandom::getrandom(&mut random).ok(); | ||
| random | ||
| } else { | ||
| *b"0123456789012345" |
There was a problem hiding this comment.
Wow, I didn't know that it is copiable. ;o
Co-authored-by: Mateusz Dahlke <39696234+Xavrax@users.noreply.github.com>
|
my bad. I see that |
|
|
||
| fn initialization_vector(&self) -> [u8; 16] { | ||
| let mut random = [0u8; AES_BLOCK_SIZE]; | ||
| getrandom::getrandom(&mut random).ok(); |
There was a problem hiding this comment.
I'm actually not sure about this. The getrandom crate's docs says this:
It is assumed that the system always provides high-quality cryptographically secure random data
Is it safe to make this assumption? On the other hand, even the rand crate, which explicitly says it provides cryptographically secure random number generators, says this:
A Rust library for random number generation, featuring secure seeding via the getrandom crate
So it isn't clear that using the rand crate would be any better. My best guess is that this will be fine for nearly all customers. However, there may be customers whose software runs in an environment (e.g. embedded hardware) that does not have access to cryptographically secure seeding. While it would be possible to allow customers to provide their own custom RNG in those cases, probably it's best that those customers just don't use this feature.
All of which is to say I am not recommending any change here.
There was a problem hiding this comment.
Yes. It looks like all suitable (no_std) CSPRNGs from this list rely on getrandom when we need to create an instance of random number generator by requesting create it from entropy – this under the hood relies on the getrandom feature.
Currently, it is hard to say what is the best solution here. With future updates, maybe we will create some deviation (from other SDKs) in the interface to let user specify closure which will generate random data for IV (and salt when we will switch to KDF).
| lib::alloc::{format, string::ToString, vec, vec::Vec}, | ||
| }; | ||
|
|
||
| type Encryptor = cbc::Encryptor<aes::Aes256>; |
There was a problem hiding this comment.
The docs for the aes crate say:
⚠️ Warning: Hazmat! This crate does not ensure ciphertexts are authentic (i.e. by using a MAC to verify ciphertext integrity), which can lead to serious vulnerabilities if used incorrectly! To avoid this, use an AEAD mode based on AES, such as AES-GCM or AES-GCM-SIV. USE AT YOUR OWN RISK!
Obviously this is not great. Solutions are to use AES-GCM, which would involve changing all the SDKs, find an AES crate which does not have this vulnerability (they exist, but they haven't been audited. At least the aes crate has been audited), or live with this issue. Another possibility would be to MAC the messages ourselves, but that's probably just as hard as switching to AES-GCM, and more error-prone.
I'm not sure what the level of vulnerability is here. We might need an actual cryptographer to weigh in.
There was a problem hiding this comment.
Before we had were advised to use AES-GCM to protect against Padding Oracles, but all encryption and decryption happens on the device (end to end) so there is no Oracle (server) to exploit. If there were servers, then yes, AES-GCM with authentication tags would help here, but it is just not our case.
There was a problem hiding this comment.
After some consultation, I think you are basically correct. Perhaps a future GCM-based cryptor, but no changes here for now.
User `AES_BLOCK_SIZE` instead of 'magic' numbers. Co-authored-by: josh-lubliner <103449628+josh-lubliner@users.noreply.github.com>
|
@pubnub-release-bot release |
pubnub-release-bot
requested review from
techwritermat and
kazydek
as code owners
|
🚀 Release successfully completed 🚀 |
feat(crypto): add crypto module
Update the crypto module structure and add enhanced AES-CBC cryptor.