Encrypt data with secret key #2803

va-an · 2024-06-03T14:13:54Z

Implements encryption and decryption functions for arbitrary textual data, as discussed in #477.

Added functions encrypt and decrypt to SecretKey.
AAD is not used since we don't have metadata like cookie names.
Functions for encryption/decryption are based on implementations from cookie-rs.
Added example with suggested usage.

the10thWiz

@SergioBenitez can weigh in, but I think this is pretty close to being ready for merge. There are a few specific points in the API I noted.

core/lib/src/config/secret_key.rs

examples/private-data/Rocket.toml

examples/private-data/src/main.rs

the10thWiz

I'm almost ready to approve these changes. @SergioBenitez should take a look, I think the implementation is solid.

core/lib/src/config/secret_key.rs

SergioBenitez · 2024-06-05T08:20:06Z

core/lib/src/config/secret_key.rs

+        let key: [u8; KEY_LEN] = self.key
+            .encryption()
+            .try_into()
+            .map_err(|_| Error::KeyLengthError)?;


This stores the key on the stack, which shouldn't be necessary. Furthermore, it reuses the encryption() key for a different operation, which we shouldn't do. Instead we should use a KDF (say HKDF) to generate a new key for this purpose using a unique info string.

I have added encryption key generation using HKDF, so the original key is no longer stored on the stack.
I'm not sure I understand about enсryption() - do we need to use a different method to obtain the secret key for KDF?

SergioBenitez · 2024-06-05T08:39:50Z

core/lib/src/config/secret_key.rs

+            .map_err(|_| Error::KeyLengthError)?;
+
+        // Create a new AES-256-GCM instance with the provided key
+        let aead = Aes256Gcm::new(GenericArray::from_slice(&key));


I think we likely want to use a more a nonce-resistant algorithm here, either AES-GCM-SIV or XChaCha20Poly1305.

Great point! Should we do the same for cookie-rs?

I am going to use XChaCha20Poly1305 because the crate chacha20poly1305 has received a security audit, and the crate aes_gcm_siv has not been audited yet.

That sounds good. cookie-rs has slightly different requirements. It may be worthwhile to change our algorithm there too, at least to aes-gcm-siv but the trade-off is different there. It's something I'm considering, but it's orthogonal to this PR.

SergioBenitez · 2024-06-05T08:42:20Z

core/lib/src/config/secret_key.rs

+        let mut nonce = [0u8; NONCE_LEN];
+        let mut rng = rand::thread_rng();
+        rng.try_fill_bytes(&mut nonce).map_err(|_| Error::NonceFillError)?;
+        let nonce = Nonce::from_slice(&nonce);


Note that you can use the generate_nonce() method to generate a properly sized nonce.

SergioBenitez · 2024-06-05T08:51:28Z

core/lib/src/config/secret_key.rs

+    ///
+    /// assert_eq!(decrypted, plaintext);
+    /// ```
+    pub fn encrypt<T: AsRef<[u8]>>(&self, value: T) -> Result<Vec<u8>, Error> {


A return type of Vec<u8> is not particularly useful. It would be nice to return something that encapsulates the Vec<u8> with convenient methods, like blake3::Hash. For example, something lIke:

struct Cipher(Vec<u8>); impl Cipher { fn from_bytes(&[u8]) -> Result<Self>; fn from_vec(Vec<u8>) -> Result<Self>; fn from_hex(&str) -> Result<Self>; fn from_base64(&str) -> Result<Self>; fn as_bytes(&self) -> &[u8]; fn into_vec(self) -> Vec<u8>; fn to_hex(&self) -> String; fn to_base64(&self) -> String; }

SergioBenitez · 2024-06-05T08:53:03Z

core/lib/src/config/secret_key.rs

+    ///
+    /// assert_eq!(decrypted, plaintext);
+    /// ```
+    pub fn encrypt<T: AsRef<[u8]>>(&self, value: T) -> Result<Vec<u8>, Error> {


It would be nice to provide a variant that allows the caller to provide associated data.

We can implement it in different ways.

change the current method by adding an optional argument for AAD:

pub fn encrypt<T: AsRef<[u8]>, A: AsRef<[u8]>>( &self, value: T, aad: Option<A> ) -> Result<Cipher, Error> {

or add separate method with additional argument (and accordingly a similar method
for decrypt):

pub fn encrypt_with_aad<T: AsRef<[u8]>, A: AsRef<[u8]>>( &self, value: T, aad: A ) -> Result<Cipher, Error> {

I prefer the second way. Despite requiring more code, it might be clearer for the user.
Which one do you like more?

SergioBenitez · 2024-06-05T09:03:48Z

core/lib/src/config/secret_key.rs

+    /// Decrypts the given encrypted data.
+    /// Extracts the nonce from the data and uses it for decryption.
+    /// Returns the decrypted Vec<u8>.
+    pub fn decrypt<T: AsRef<[u8]>>(&self, encrypted: T) -> Result<Vec<u8>, Error> {


This would then take a Cipher, which is pre-validated.

SergioBenitez · 2024-06-07T22:51:39Z

core/lib/src/config/secret_key.rs

 };
 use cookie::Key;
 use serde::{de, ser, Deserialize, Serialize};

 use crate::request::{Outcome, Request, FromRequest};

-const NONCE_LEN: usize = 12;
+const NONCE_LEN: usize = 24; // 192-bit


I don't think we need this anymore. There's NonceSize.

SergioBenitez · 2024-06-07T22:53:13Z

core/lib/src/config/secret_key.rs

 };
 use cookie::Key;
 use serde::{de, ser, Deserialize, Serialize};

 use crate::request::{Outcome, Request, FromRequest};

-const NONCE_LEN: usize = 12;
+const NONCE_LEN: usize = 24; // 192-bit
 const KEY_LEN: usize = 32;


Same with this. We should be able to get it from the Key and ConstArrayLen.

SergioBenitez · 2024-06-07T22:55:08Z

core/lib/src/config/secret_key.rs

+            .map_err(|_| Error::KeyLengthError)?;
+
+        // Create a new AES-256-GCM instance with the provided key
+        let aead = Aes256Gcm::new(GenericArray::from_slice(&key));


That sounds good. cookie-rs has slightly different requirements. It may be worthwhile to change our algorithm there too, at least to aes-gcm-siv but the trade-off is different there. It's something I'm considering, but it's orthogonal to this PR.

va-an · 2024-06-08T18:27:56Z

Rebased on master.

the10thWiz · 2024-06-10T23:50:18Z

There has been an issue with CI on Windows, so you will need to merge the latest commit from master before CI will pass.

va-an · 2024-06-11T19:43:25Z

There has been an issue with CI on Windows, so you will need to merge the latest commit from master before CI will pass.

Understood, I will do it after I finish making changes based on the comments.
Thanks!

the10thWiz

I think is getting closer to a complete PR, but there are still a number of changes that need to be made.

the10thWiz · 2024-07-02T00:14:42Z

core/lib/Cargo.toml

+chacha20poly1305 = "0.10.1"
+hkdf = "0.12.4"
+sha2 = "0.10.8"
+base64 = "0.22.1"
+hex = "0.4.3"


All of these should probably be feature-gated behind the secrets feature.

the10thWiz · 2024-07-02T00:16:09Z

core/lib/src/config/secret_key.rs

+        if encrypted.len() <= nonce_len {
+            return Err(Error::EncryptedDataLengthError);
+        }


This should not need to be checked here. Rather, it should be checked when Cipher is constructed.

the10thWiz · 2024-07-02T00:19:40Z

core/lib/src/config/secret_key.rs

+    /// Create a `Cipher` from its raw bytes representation.
+    pub fn from_bytes(bytes: &[u8]) -> Self {
+        Cipher(bytes.to_vec())
+    }
+
+    /// Create a `Cipher` from a vector of bytes.
+    pub fn from_vec(vec: Vec<u8>) -> Self {
+        Cipher(vec)
+    }
+
+    /// Create a `Cipher` from a hex string.
+    pub fn from_hex(hex: &str) -> Result<Self, Error> {
+        let decoded  = hex::decode(hex).map_err(|_| Error::HexDecodeError)?;
+        Ok(Cipher(decoded))
+    }
+
+    /// Create a `Cipher` from a base64 string.
+    pub fn from_base64(base64: &str) -> Result<Self, Error> {
+        let decoded = URL_SAFE.decode(base64).map_err(|_| Error::Base64DecodeError)?;
+        Ok(Cipher(decoded))
+    }


All of these methods should validate that the length is longer than the Nonce, and that the length (minus the Nonce) is a multiple of the block size. Ideally, the only error that should be caught when decrypting the value is that the value was not created by encrypting with the same secret key.

the10thWiz · 2024-07-02T00:21:03Z

core/lib/src/config/secret_key.rs

+/// let data = b"some encrypted data";
+/// let cipher = Cipher::from_bytes(data);
+/// ```
+///
+/// Converting a `Cipher` to a hexadecimal string:
+/// ```
+/// let hex = cipher.to_hex();
+/// ```
+///
+/// Creating a `Cipher` from a base64 string:
+/// ```
+/// let base64_str = "c29tZSBlbmNyeXB0ZWQgZGF0YQ==";
+/// let cipher = Cipher::from_base64(base64_str).unwrap();
+/// ```
+///
+/// Converting a `Cipher` back to bytes:
+/// ```
+/// let bytes = cipher.as_bytes();


Many of these doc-tests fail. Use cargo test --doc to run them locally.

the10thWiz · 2024-07-02T00:26:58Z

core/lib/src/config/secret_key.rs

+/// let bytes = cipher.as_bytes();
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Cipher(Vec<u8>);


To make Cipher more useful, it could implement FromForm, FromParam, FromSegment, and FromData. They should either check the format (which may not be possible, since hex could be re-interpreted as base-64), or always use one specific encoding (and provide a method like encode that encodes it using the same encoding).

Similarly, we could implement Serialize and Deserialize if the serde feature is enabled.

va-an · 2024-07-07T09:28:07Z

@the10thWiz Thanks for the thorough review!
I merged the changes from master that you mentioned and moved the dependencies to the "secrets" feature.
I will address the remaining comments a bit later.

SergioBenitez · 2024-08-09T00:05:26Z

@va-an Checking in. Any update?

the10thWiz reviewed Jun 3, 2024

View reviewed changes

va-an requested a review from the10thWiz June 4, 2024 18:41

the10thWiz requested changes Jun 5, 2024

View reviewed changes

core/lib/src/config/secret_key.rs Outdated Show resolved Hide resolved

SergioBenitez requested changes Jun 5, 2024

View reviewed changes

SergioBenitez reviewed Jun 5, 2024

View reviewed changes

SergioBenitez requested changes Jun 7, 2024

View reviewed changes

va-an added 9 commits June 8, 2024 22:21

encrypt/decrypt data with secret key

ab6c711

change function signatures

b33f56a

delete the secret key from the example configs

4bd058b

change error types

ffea9d4

fix doc-comments

8278683

add tests

ac98764

use handy method for generate nonce

e08f9f9

use XChaCha20Poly1305 instead of Aes256Gcm

4163474

use hkdf

4d81f62

va-an force-pushed the encrypt-data-with-secret-key branch from 79adcb1 to 4d81f62 Compare June 8, 2024 18:22

return Cipher from encrypt

3c67b97

use Cipher struct in decrypt method

bf5d1cd

va-an requested a review from SergioBenitez June 26, 2024 07:50

the10thWiz requested changes Jul 2, 2024

View reviewed changes

va-an added 2 commits July 7, 2024 13:02

Merge branch 'master' into encrypt-data-with-secret-key

5296e3f

move dependencies to secrets feature

bba0d6c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encrypt data with secret key #2803

Encrypt data with secret key #2803

va-an commented Jun 3, 2024 •

edited

Loading

the10thWiz left a comment

the10thWiz left a comment

SergioBenitez Jun 5, 2024 •

edited

Loading

va-an Jun 15, 2024

SergioBenitez Jun 5, 2024

va-an Jun 7, 2024

va-an Jun 7, 2024

SergioBenitez Jun 7, 2024

SergioBenitez Jun 5, 2024

va-an Jun 15, 2024

SergioBenitez Jun 5, 2024

SergioBenitez Jun 5, 2024

va-an Jun 13, 2024

SergioBenitez Jun 5, 2024

SergioBenitez Jun 7, 2024

SergioBenitez Jun 7, 2024

SergioBenitez Jun 7, 2024

va-an commented Jun 8, 2024

the10thWiz commented Jun 10, 2024

va-an commented Jun 11, 2024

the10thWiz left a comment

the10thWiz Jul 2, 2024

the10thWiz Jul 2, 2024

the10thWiz Jul 2, 2024

the10thWiz Jul 2, 2024

the10thWiz Jul 2, 2024

the10thWiz Jul 2, 2024

va-an commented Jul 7, 2024

SergioBenitez commented Aug 9, 2024

Encrypt data with secret key #2803

Are you sure you want to change the base?

Encrypt data with secret key #2803

Conversation

va-an commented Jun 3, 2024 • edited Loading

the10thWiz left a comment

Choose a reason for hiding this comment

the10thWiz left a comment

Choose a reason for hiding this comment

SergioBenitez Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

va-an commented Jun 8, 2024

the10thWiz commented Jun 10, 2024

va-an commented Jun 11, 2024

the10thWiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

va-an commented Jul 7, 2024

SergioBenitez commented Aug 9, 2024

va-an commented Jun 3, 2024 •

edited

Loading

SergioBenitez Jun 5, 2024 •

edited

Loading