Skip to content

Commit

Permalink
fix: improve key handling (#4994)
Browse files Browse the repository at this point in the history
Description
---
Updates the use of `CoreTransactionAEADKey` and `CommsNoiseKey` to be populated in place. Switches `CommsNoiseKey` to use a `SafeArray` under the hood.

Extends the work of [PR 4967](#4967).

Motivation and Context
---
Earlier work updates the use of keys for value encryption and Noise. Value encryption keys use a `SafeArray` type in a `Hidden` wrapper, and Noise keys use a `Hidden` array. However, in both cases, a copy of the hash output used to populate the keys is left in memory.

This work mitigates the problem. Because the hashing API now supports in-place output finalization via `finalize_into` and `finalize_into_reset`, we can populate keys directly by mutable reference.

It also renames `CoreTransactionAEADKey` to `EncryptedValueKey` for clarity, and switches `CommsNoiseKey` to be a `SafeArray` under the hood. There is [discussion](#4967 (comment)) on whether `CommsNoiseKey` needs to be a `SafeArray`; while the reasoning makes sense, I still think it's good practice to take advantage of the benefits of `SafeArray` for array-like key types unless there's a compelling performance reason otherwise. Discussion on this is welcome!

How Has This Been Tested?
---
Existing tests pass.
  • Loading branch information
AaronFeickert authored Dec 5, 2022
1 parent 5548104 commit f069b14
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@ use chacha20poly1305::{
KeyInit,
Nonce,
};
use digest::generic_array::GenericArray;
use digest::{generic_array::GenericArray, FixedOutput};
use serde::{Deserialize, Serialize};
use tari_common_types::types::{Commitment, PrivateKey};
use tari_crypto::{hash::blake2::Blake256, hashing::DomainSeparatedHasher};
use tari_utilities::{safe_array::SafeArray, ByteArray, ByteArrayError};
use thiserror::Error;
use zeroize::Zeroize;

use super::{CoreTransactionAEADKey, AEAD_KEY_LEN};
use super::EncryptedValueKey;
use crate::transactions::{tari_amount::MicroTari, TransactionKdfDomain};

const SIZE: usize = 24;
Expand Down Expand Up @@ -117,16 +117,13 @@ impl EncryptedValue {
}
}

// Generate a ChaCha20-Poly1305 key from an ECDH shared secret and commitment using Blake2b
fn kdf_aead(shared_secret: &PrivateKey, commitment: &Commitment) -> CoreTransactionAEADKey {
let output = DomainSeparatedHasher::<Blake256, TransactionKdfDomain>::new_with_label("encrypted_value")
.chain(shared_secret.as_bytes())
// Generate a ChaCha20-Poly1305 key from a private key and commitment using Blake2b
fn kdf_aead(private_key: &PrivateKey, commitment: &Commitment) -> EncryptedValueKey {
let mut aead_key = EncryptedValueKey::from(SafeArray::default());
DomainSeparatedHasher::<Blake256, TransactionKdfDomain>::new_with_label("encrypted_value")
.chain(private_key.as_bytes())
.chain(commitment.as_bytes())
.finalize();

let default_array = SafeArray::<u8, AEAD_KEY_LEN>::default();
let mut aead_key = CoreTransactionAEADKey::from(default_array);
aead_key.reveal_mut().copy_from_slice(&output.as_ref()[..AEAD_KEY_LEN]);
.finalize_into(GenericArray::from_mut_slice(aead_key.reveal_mut()));

aead_key
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub const MAX_TRANSACTION_RECIPIENTS: usize = 15;
pub(crate) const AEAD_KEY_LEN: usize = std::mem::size_of::<Key>();

// Type for hiding aead key encryption
hidden_type!(CoreTransactionAEADKey, SafeArray<u8, AEAD_KEY_LEN>);
hidden_type!(EncryptedValueKey, SafeArray<u8, AEAD_KEY_LEN>);

//---------------------------------------- Crate functions ----------------------------------------------------//

Expand Down
18 changes: 10 additions & 8 deletions comms/core/src/noise/crypto_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

use digest::{generic_array::GenericArray, FixedOutput};
use rand::rngs::OsRng;
use snow::{
params::{CipherChoice, DHChoice, HashChoice},
Expand All @@ -32,8 +33,9 @@ use tari_crypto::{
keys::{PublicKey, SecretKey},
tari_utilities::ByteArray,
};
use tari_utilities::safe_array::SafeArray;

use super::{CommsNoiseKey, NOISE_KEY_LEN};
use super::CommsNoiseKey;
use crate::types::{CommsCoreHashDomain, CommsDHKE, CommsPublicKey, CommsSecretKey};

macro_rules! copy_slice {
Expand Down Expand Up @@ -67,12 +69,12 @@ impl CryptoResolver for TariCryptoResolver {
}

fn noise_kdf(shared_key: &CommsDHKE) -> CommsNoiseKey {
let hasher = DomainSeparatedHasher::<Blake256, CommsCoreHashDomain>::new_with_label("noise.dh");
let mut comms_noise_kdf = CommsNoiseKey::from([0u8; NOISE_KEY_LEN]);
comms_noise_kdf
.reveal_mut()
.copy_from_slice(hasher.chain(shared_key.as_bytes()).finalize().as_ref());
comms_noise_kdf
let mut comms_noise_key = CommsNoiseKey::from(SafeArray::default());
DomainSeparatedHasher::<Blake256, CommsCoreHashDomain>::new_with_label("noise.dh")
.chain(shared_key.as_bytes())
.finalize_into(GenericArray::from_mut_slice(comms_noise_key.reveal_mut()));

comms_noise_key
}

#[derive(Default)]
Expand Down Expand Up @@ -129,7 +131,7 @@ impl Dh for CommsDiffieHellman {
mod test {
use snow::Keypair;

use super::*;
use super::{super::NOISE_KEY_LEN, *};
use crate::noise::config::NOISE_IX_PARAMETER;

fn build_keypair() -> Keypair {
Expand Down
4 changes: 2 additions & 2 deletions comms/core/src/noise/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ pub use error::NoiseError;

mod socket;
pub use socket::NoiseSocket;
use tari_utilities::{hidden_type, Hidden};
use tari_utilities::{hidden_type, safe_array::SafeArray, Hidden};
use zeroize::Zeroize;

pub(crate) const NOISE_KEY_LEN: usize = 32;

hidden_type!(CommsNoiseKey, [u8; NOISE_KEY_LEN]);
hidden_type!(CommsNoiseKey, SafeArray<u8, NOISE_KEY_LEN>);

0 comments on commit f069b14

Please sign in to comment.