Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 48 additions & 15 deletions src/util/storable_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@ use std::io;
use std::io::{Error, ErrorKind};

/// [`StorableBuilder`] is a utility to build and deconstruct [`Storable`] objects.
///
/// It provides client-side Encrypt-then-MAC using ChaCha20-Poly1305.
pub struct StorableBuilder<T: EntropySource> {
data_encryption_key: [u8; 32],
entropy_source: T,
}

impl<T: EntropySource> StorableBuilder<T> {
/// Constructs a new instance.
pub fn new(data_encryption_key: [u8; 32], entropy_source: T) -> StorableBuilder<T> {
Self { data_encryption_key, entropy_source }
pub fn new(entropy_source: T) -> StorableBuilder<T> {
Self { entropy_source }
}
}

Expand All @@ -34,18 +34,21 @@ const CHACHA20_CIPHER_NAME: &'static str = "ChaCha20Poly1305";
impl<T: EntropySource> StorableBuilder<T> {
/// Creates a [`Storable`] that can be serialized and stored as `value` in [`PutObjectRequest`].
///
/// Uses ChaCha20 for encrypting `input` and Poly1305 for generating a mac/tag.
/// Uses ChaCha20 for encrypting `input` and Poly1305 for generating a mac/tag with associated
/// data `aad` (usually the storage key).
///
/// Refer to docs on [`Storable`] for more information.
///
/// [`PutObjectRequest`]: crate::types::PutObjectRequest
pub fn build(&self, input: Vec<u8>, version: i64) -> Storable {
pub fn build(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we could be opinionated here for future writes and have the AAD commit to:

  1. The key-value key (lol) [fixes the issue mentioned in pr]
  2. store_id [fixes potential multi-tenancy issues]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: 2.:

  1. The key-value key (lol) [fixes the issue mentioned in pr]

Hmm, that might be an option, but (especially for backwards compat) the current approach where the 'user' determines the aad (given that they also build the actual storable) might be more flexible?

  1. store_id [fixes potential multi-tenancy issues]

I guess we also could consider committing to the store id, but it would mean it can never change, e.g., that services can't ever move data between stores for the user.

Also, as noted below, I doubt the described replay attacks can be fully mitigated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On whether we should commit to store_id:

Since a VSS store is:

pub struct VssStore {
	client: VssClient<CustomRetryPolicy>,
	store_id: String,
	runtime: Arc<Runtime>,
	data_encryption_key: [u8; KEY_LENGTH],
	key_obfuscator: KeyObfuscator,
}

We already have the notion that each store_id should have a unique DEK.

So as long as the user uses a different DEK for each store_id, the server won't be able to move data to a different store_id without being transparent with the user.

If I understand things correctly here, I would rather not commit to the store_id in the aad, and suggest users use a different DEK for each store_id - which seems to fit well with the current VssStore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for committing to the key-value key, yea I think letting the user set the aad so that we have an easier time with backwards compat makes the most sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have the notion that each store_id should have a unique DEK.

Huh, why? Users currently can change the store_id when setting up LDK with VSS, without changing their entropy, and the store_id doesn't influence how the DEK is derived?

If I understand things correctly here, I would rather not commit to the store_id in the aad, and suggest users use a different DEK for each store_id - which seems to fit well with the current VssStore.

Right, we could change the DEK derivation scheme to include the store_id, but a) we'll need to figure out how to maintain backwards compatibility for the 'legacy' derivation and b) this would mean that we can't easily migrate between stores/store ids. Might be debatable if that's what we want, but we'd def. block off that road.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users currently can change the store_id when setting up LDK with VSS, without changing their entropy, and the store_id doesn't influence how the DEK is derived?

Ah yes you are right.

this would mean that we can't easily migrate between stores/store ids. Might be debatable if that's what we want, but we'd def. block off that road.

Should migrations between store_id of this kind be handled by the user, not by the server ? Ie list all keys, get all the values from the old store, and put them in the new store.

Right, we could change the DEK derivation scheme to include the store_id

In that case I would rather just commit to the store_id in the aad instead of including it in the DEK.

we'll need to figure out how to maintain backwards compatibility for the 'legacy' derivation

1- How important is it to maintain backwards compat at this point ? This feature is alpha after all.
2- If we want to maintain backwards compat, could we create a migration routine on startup that triggers for old VSS stores, and transitions them to the new store + scheme ? We'd have to support both schemes at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should migrations between store_id of this kind be handled by the user, not by the server ? Ie list all keys, get all the values from the old store, and put them in the new store.

Yes, that sounds reasonable. Just saying we're blocking off the other way and need to take that step consciously. Committing to the store id also manifests that it needs to remain the users' choice I believe?

How important is it to maintain backwards compat at this point ? This feature is alpha after all.

'Alpha' or not, we have users in production with it. So at the very least we need to offer some migration path for them.

2- If we want to maintain backwards compat, could we create a migration routine on startup that triggers for old VSS stores, and transitions them to the new store + scheme ? We'd have to support both schemes at once.

I think this discussion is better had over on the LDK Node PR, but I was thinking to introduce a vss_version (or similar) key that is read first thing and default to 0 if it's absent.

&self, input: Vec<u8>, version: i64, data_encryption_key: &[u8; 32], aad: &[u8],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the aad is required to have a specific (set of?) lengths or we panic, we shouldn't expose a generic any-length API publicly as a result, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the aad is required to have a specific (set of?) lengths or we panic, we shouldn't expose a generic any-length API publicly as a result, IMO.

No, AFAICT that's only the key and the nonce?:

			assert!(key.len() == 16 || key.len() == 32);
			assert!(nonce.len() == 12);

For the aad we even include the length in the mac after padding has been added:

			self.mac.input(&self.aad_len.to_le_bytes());
			self.mac.input(&(self.data_len as u64).to_le_bytes());

Or maybe I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, it might still be debatable if we simply want to take the storage_key as an argument here and prescribe the specific commitment scheme. Just seemed a bit more flexible to leave it to the user, in case they wanted not to do it for some reason or needed something else committed to.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops sorry no you're right, I was thinking of the nonce. I think this API is fine.

) -> Storable {
let mut nonce = vec![0u8; 12];
self.entropy_source.fill_bytes(&mut nonce[4..]);

let mut data_blob = PlaintextBlob { value: input, version }.encode_to_vec();

let mut cipher = ChaCha20Poly1305::new(&self.data_encryption_key, &nonce, &[]);
let mut cipher = ChaCha20Poly1305::new(data_encryption_key, &nonce, aad);
let mut tag = vec![0u8; 16];
cipher.encrypt_inplace(&mut data_blob, &mut tag);
Storable {
Expand All @@ -62,10 +65,14 @@ impl<T: EntropySource> StorableBuilder<T> {
/// corresponding version as stored at the time of [`PutObjectRequest`].
///
/// [`PutObjectRequest`]: crate::types::PutObjectRequest
pub fn deconstruct(&self, mut storable: Storable) -> io::Result<(Vec<u8>, i64)> {
let encryption_metadata = storable.encryption_metadata.unwrap();
pub fn deconstruct(
&self, mut storable: Storable, data_encryption_key: &[u8; 32], aad: &[u8],
) -> io::Result<(Vec<u8>, i64)> {
let encryption_metadata = storable
.encryption_metadata
.ok_or_else(|| Error::new(ErrorKind::InvalidData, "Invalid Metadata"))?;
let mut cipher =
ChaCha20Poly1305::new(&self.data_encryption_key, &encryption_metadata.nonce, &[]);
ChaCha20Poly1305::new(data_encryption_key, &encryption_metadata.nonce, aad);

cipher
.decrypt_inplace(&mut storable.data, encryption_metadata.tag.borrow())
Expand Down Expand Up @@ -97,16 +104,42 @@ mod tests {
let test_entropy_provider = TestEntropyProvider;
let mut data_key = [0u8; 32];
test_entropy_provider.fill_bytes(&mut data_key);
let storable_builder = StorableBuilder {
data_encryption_key: data_key,
entropy_source: test_entropy_provider,
};
let storable_builder = StorableBuilder::new(test_entropy_provider);
let expected_data = b"secret".to_vec();
let expected_version = 8;
let storable = storable_builder.build(expected_data.clone(), expected_version);
let aad = b"A";
let storable =
storable_builder.build(expected_data.clone(), expected_version, &data_key, aad);

let (actual_data, actual_version) = storable_builder.deconstruct(storable).unwrap();
let (actual_data, actual_version) =
storable_builder.deconstruct(storable, &data_key, aad).unwrap();
assert_eq!(actual_data, expected_data);
assert_eq!(actual_version, expected_version);
}

#[test]
fn decrypt_key_mismatch_fails() {
let test_entropy_provider = TestEntropyProvider;
let mut data_key = [0u8; 32];
test_entropy_provider.fill_bytes(&mut data_key);
let storable_builder = StorableBuilder::new(test_entropy_provider);

let expected_data_a = b"secret_a".to_vec();
let expected_version_a = 8;
let aad_a = b"A";
let storable_a =
storable_builder.build(expected_data_a.clone(), expected_version_a, &data_key, aad_a);

let expected_data_b = b"secret_b".to_vec();
let expected_version_b = 8;
let aad_b = b"B";
let storable_b =
storable_builder.build(expected_data_b.clone(), expected_version_b, &data_key, aad_b);

let (actual_data, actual_version) =
storable_builder.deconstruct(storable_a, &data_key, aad_a).unwrap();
assert_eq!(actual_data, expected_data_a);
assert_eq!(actual_version, expected_version_a);
assert!(storable_builder.deconstruct(storable_b, &data_key, aad_a).is_err());
}
}