feat: adds support for encryption by brunocalza · Pull Request #109 · recallnet/rust-recall

brunocalza · 2024-10-31T19:39:58Z

Summary

This PR adds support for encrypting files with encrypting key provided by the client

Closes #110

@carsonfarmer marking you as a reviewer. But maybe you can review more from a high-level protocol/cryptography/correctness perspective?
@dtbuchholz marking you as a reviewer. But maybe more from a CLI/SDK methods DX perspective?
@avichalp marking you as reviewer. Maybe more from code and Rust perspective? You already gave a lot of input in the cryptographic stuff

cc @andrewxhill @sanderpick @rohhan @oed

Pre-requisites for reviewing

Have an understanding of the DARE format and our implementation of it (https://github.com/hokunet/dare)
Have an understanding of Envelope Encryption and best practices for DEKs and KEKs

Implementation overview

At a high level this is everything implemented in this PR:

Adds the options to encrypt objects by providing a 256-bit base64 encoded key in add_from_path and add_reader methods
If encryption is requested, we wrap the reader with encryption logic
- The encryption logic can be grasped by having an understanding of the pre-requisites and reading Encryption POC using MinIO's KES (although this is not about KMS and S3, the core of the encryption logic is there)
Adds the option to decrypt objects in the get method
If decryption is requested, we wrap the writer with decryption logic
Enhances the CLI to account for encryption and decryption

# encrypting with base64-encoded encryption key
hoku bucket add -a t2h4egii4s7p76w5fuwg6srrefnjanhkxuxiew6ha --enc-c AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= --key hello.txt $(PWD)/hello.txt

# without decrypting
hoku bucket get -a t2h4egii4s7p76w5fuwg6srrefnjanhkxuxiew6ha  hello.txt 

# decrypting
hoku bucket get -a t2h4egii4s7p76w5fuwg6srrefnjanhkxuxiew6ha --enc-c AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= hello.txt

# decrypting with range
hoku bucket get -a t2h4egii4s7p76w5fuwg6srrefnjanhkxuxiew6ha --enc-c AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= --range 2-4 hello.txt

Adds support for range queries over encrypted data
Adds integration tests that require a network running

Review orientation

Start at sdk/src/machine/bucket.rs, which is the crux of the SDK methods and we how they were altered to support encryption
Start at encryption, then go to decryption
Play with the CLI
Try writing a code as an example of how to add an encrypted object and how to retrieve it

Additional notes

This work was built on top of learnings from Encryption POC using MinIO's KES (and wip: adds encryption recall-s3#14) but has nothing to do with KMS or S3.
This build fails because it has no access to the dare repo. Can someone set those deploys keys for me?
We need to implement DARE v2

only for customer supplied key Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza · 2024-10-31T19:54:23Z

cli/src/machine/bucket.rs

    tx_args: TxArgs,
+    /// Encrypt/Decrypt objects using 256-bit encryption keys. Formats: RawBase64.
+    #[arg(long)]
+    enc_c: Option<String>,


A new option to put comand is added (--enc-c). It's through this option that the client provides the encryption key. It must be a 256-bit base64-encoded key

brunocalza · 2024-10-31T19:54:58Z

cli/src/machine/bucket.rs

    height: FvmQueryHeight,
+    /// Encrypt/Decrypt objects using 256-bit encryption keys. Formats: RawBase64.
+    #[arg(long)]
+    enc_c: Option<String>,


A new option to get comand is added (--enc-c). It's through this option that the client provides the encryption key to decrypt the object. It must be the same used for encryption.

--enc-c sounds a bit confusing name for an argument to me. is that a convention for encryption CLIs?

Just using the same name used by minio's client. I don't think there's a convention, aws cli uses --sse-c and google cloud cli uses --encryption-key

brunocalza · 2024-10-31T20:01:19Z

sdk/src/encryption/decryptor.rs

+}
+
+#[pin_project]
+pub struct DecryptWriter<W: AsyncWrite + Unpin> {


This object is probably very hard to understand. And it took me a while to get it right (mostly because of my lack of understanding of Pin).

In essence, it decorates a writer that implements AsyncWrite. The idea is that as the data is written to this writer we intercept it, decrypt it, and fill the inner writer.

This is the function that does the wrapping logic.

You can pass a Filter to this object in case of a Range query. So, if you want to apply a range query on encrypted data you use with_filter, if not you use new

The implementation follows a state machine with the following states:

ReadingHeader: the state where we need to grab the next 16B of the payload and decode the header

Decrypting: the state where grab the rest of the package 65 KiB (ciphertext) + 16B (tag), decrypt it and fill the decrypted buffer

Writing: the state where we asynchronously write to the inner writer`

The Filter struct means:

offset: any byte less than offset should be ignored and not written to the inner

length: How many bytes should we include from the offset? Anything more than offset + length should be ignored

consumed: This is just to indicate how many bytes we have already consumed (or peeked, or considered) from the beginning

brunocalza · 2024-10-31T20:14:22Z

sdk/src/encryption/decryptor.rs

+        }
+    }
+
+    fn filter_bytes<'a>(&mut self, plaintext: &'a [u8]) -> &'a [u8] {


Kind of a complicated method, but in essence, we are trying to figure out how much of the plaintext chunk (decrypted content chunk) falls within offset + length.

consumed is used as a pointer (not in a programming language sense), and we do math based if we have reached offset or if we have passed offset+ length. We use remaining to indicate how many bytes we still need to write. After remaining reaches 0, we ignore everything

I would review this last, also take a look at the test

brunocalza · 2024-10-31T20:19:39Z

sdk/src/encryption/decryptor.rs

+                        .map_err(|err| Error::new(ErrorKind::InvalidData, err.to_string()))?;
+
+                    #[allow(clippy::drop_non_drop)]
+                    drop(this);


this is ugly, but I cannot call the method filter_bytes on this or self for some reason. The borrow checker complains. There's something about it I still don't understand, so I came up with this hack.

brunocalza · 2024-10-31T20:46:52Z

sdk/src/encryption/key.rs

+        self.domain.clone()
+    }
+
+    pub fn unseal(&self, kek: String, object_path: &str) -> anyhow::Result<ObjectKey> {


This is how we decrypt ObjectKey

brunocalza · 2024-10-31T20:49:06Z

sdk/src/encryption/object.rs

+    fn size_decrypted(&self) -> u64;
+}
+
+impl EncryptedObjectExt for Object {


We're extending Hoku Object to add some functionality:

Is this object encrypted?

Which kind of encryption was used (client or kms) note: kms is not really implemented

Give me the sealed object key

Give me the size of the decrypted object

This makes the client code cleaner

brunocalza · 2024-10-31T20:50:26Z

sdk/src/encryption/sse_c.rs

+    let iv = sealed_object_key.iv_as_string()?;
+    let algorithm = sealed_object_key.algorithm();
+
+    let mut metadata = HashMap::new();


we need to store these metadata for decryption

brunocalza · 2024-10-31T20:51:28Z

sdk/src/encryption/sse_c.rs

+use std::collections::HashMap;
+use tokio::io::AsyncRead;
+
+pub fn encrypt_reader<R: AsyncRead>(


the encryption logic

derives object key

wraps reader

encrypts object key

stores metadata

brunocalza · 2024-10-31T20:55:50Z

sdk/src/range.rs

+    end: Option<u64>,
+}
+
+impl Range {


This object is responsible for range logic

For an unencrypted object, there's no logic. We pass to Hoku whatever the user sent. For encrypted, we need to transform to a new range because encrpyted content is bigger, and also i need to retrieve the entire package to decrypt even if part of it is not withing the range. Check this test to understand this

avichalp · 2024-11-10T19:32:04Z

sdk/examples/bucket_add.rs

    let mut read_file = tokio::fs::File::open(&obj_path).await?;
    let mut contents = vec![0; 10];
-    read_file.read(&mut contents).await?;
+    let _ = read_file.read(&mut contents).await?;


you may not need let _, if you just unwrap here. probably okay to unwrap in the example code

carsonfarmer

Wow @brunocalza. This is a serious PR. I've done two passes now. I'm not 100% confident I understand all that is going on, but it looks correct from a high-level. I'm marking approved, so as not to slow things down. But I will likely want to do another pass over this to double-check nonce use etc.

carsonfarmer · 2024-11-12T18:48:42Z

sdk/src/encryption/encryptor.rs

+        }
+
+        // Encrypt the chunk and store it in the buffer
+        // TODO: handle error


Do you want to do this before initial merge? Or can we wait? It seems reasonable to panic here... but you might just have to wrap this error.

Better handling it now

carsonfarmer · 2024-11-12T18:54:04Z

sdk/src/encryption/object.rs

+    fn size_decrypted(&self) -> u64;
+}
+
+impl EncryptedObjectExt for Object {


carsonfarmer · 2024-11-13T16:14:17Z

This build fails because it has no access to the dare repo. Can someone set those deploys keys for me?

Who do we need to do this for @brunocalza?

avichalp · 2024-11-16T09:06:09Z

@brunocalza: added the deploy key and secret in the repos. the build is still failing due to some lint issue but the keys should work now: #124

Add deploy key for DARE repo

brunocalza · 2024-11-27T22:26:40Z

holding this off for a bit until we have more clarity when we want to land this feature

feat: adds support for encryption

04ba518

only for customer supplied key Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>

brunocalza self-assigned this Oct 31, 2024

brunocalza commented Oct 31, 2024

View reviewed changes

brunocalza requested review from avichalp, carsonfarmer and dtbuchholz October 31, 2024 21:31

brunocalza marked this pull request as ready for review October 31, 2024 21:31

avichalp reviewed Nov 10, 2024

View reviewed changes

carsonfarmer approved these changes Nov 12, 2024

View reviewed changes

Add deploy key for DARE repo

ed3ff69

Merge pull request #124 from hokunet/avichalp/fix-ci

2639f16

Add deploy key for DARE repo

brunocalza marked this pull request as draft November 27, 2024 22:27

sanderpick added the archived Archived for now label Mar 12, 2025

Conversation

brunocalza commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Pre-requisites for reviewing

Implementation overview

Review orientation

Additional notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carsonfarmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carsonfarmer commented Nov 13, 2024

Uh oh!

avichalp commented Nov 16, 2024

Uh oh!

brunocalza commented Nov 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brunocalza commented Oct 31, 2024 •

edited

Loading