New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: service for encrypting value #4225

Merged

stringhandler merged 1 commit into tari-project:development from therustmonk:service_for_ecrypting_value

Jul 1, 2022

Contributor

therustmonk commented Jun 22, 2022 •

edited

Loading

Description

The PR implements encryption of the EncryptedValue type.
The new methods encrypt_value and decrypt_value added to the OutputManagerService (handle).
Implemented wallet_ffi methods: encrypted_value_encrypt and encrypted_value_decrypt

Motivation and Context

The part of the BP+ feature.

How Has This Been Tested?

Unit tests + CI

therustmonk force-pushed the service_for_ecrypting_value branch from 8b4e42f to aa2d29e Compare

June 22, 2022 23:39

therustmonk requested a review from hansieodendaal

June 22, 2022 23:40

stringhandler reviewed

View reviewed changes

base_layer/core/src/transactions/encryption_service.rs Outdated

		@@ -0,0 +1,70 @@
		// Copyright 2022, The Tari Project

Collaborator

stringhandler Jun 23, 2022

I'd prefer this file to be under core/src/services

base_layer/core/src/transactions/encryption_service.rs Outdated

+                      Self {}
+                  }
+                  pub fn encrypt_value(&self, value: MicroTari) -> EncryptedValue {

Collaborator

stringhandler Jun 23, 2022

Probably needs a secret for the encryption as wel

therustmonk force-pushed the service_for_ecrypting_value branch from aa2d29e to 59ebe59 Compare

June 23, 2022 21:42

hansieodendaal reviewed

View reviewed changes

Contributor

hansieodendaal left a comment

Hi @deniskolodin, some comments below. I think the required functionality is shaping nicely.

Having looked at Aaron's scanning poc together yesterday I believe we can simplify this to follow your original implementation philosophy without the need for a service. An encryption service in the true sense would need to offer more than just the value encryption.

base_layer/wallet_ffi/src/lib.rs Outdated

                   if encrypted_value.is_null() {
                       error = LibWalletError::from(InterfaceError::NullError("encrypted_value".to_string())).code;
                       ptr::swap(error_out, &mut error as *mut c_int);
                       return 0;
                   }
-                  let encrypted_value = (*encrypted_value).clone();
-                  encrypted_value.todo_decrypt() as c_ulonglong
+                  let value = (*wallet).wallet.encryption_service.decrypt_value(&*encrypted_value);

Contributor

hansieodendaal Jun 24, 2022

This method will throw an error if the value cannot be decrypted, which must be handled here

base_layer/wallet_ffi/src/lib.rs Outdated

Comment on lines 7840 to 7845

+                          let encryption_service = EncryptionServiceHandle::new();
+                          // TODO: Commitment? Encryption Key?
+                          let encryption_key = todo!();
+                          let commitment = todo!();
+                          let expected_encrypted_value =
+                              encryption_service.encrypt_value(&encryption_key, &commitment, amount.into());

Contributor

hansieodendaal Jun 24, 2022

This is not needed, as it is sufficient to test the round trip with encrypted_value_encrypt and encrypted_value_decrypt

base_layer/core/src/services/encryption_service.rs Outdated

+                      EncryptedValue(data)
+                  }
+                  pub fn decrypt_value(&self, value: &EncryptedValue) -> MicroTari {

Contributor

hansieodendaal Jun 24, 2022

This method should throw an error if the value cannot be decrypted, as per Aaron's scanning poc

base_layer/core/src/services/encryption_service.rs Outdated

+                      Self {}
+                  }
+                  pub fn encrypt_value(&self, key: &PrivateKey, commitment: &Commitment, value: MicroTari) -> EncryptedValue {

Contributor

hansieodendaal Jun 24, 2022

The encryption_key should be passed into this function

base_layer/core/src/services/encryption_service.rs Outdated

+                      EncryptedValue(data)
+                  }
+                  pub fn decrypt_value(&self, value: &EncryptedValue) -> MicroTari {

Contributor

hansieodendaal Jun 24, 2022

The encryption_key should be passed into this function

base_layer/core/src/services/encryption_service.rs Outdated

+              // TODO: Rename to `EncryptionFactory`
+              #[derive(Debug, Clone)]
+              pub struct EncryptionServiceHandle {}

Contributor

hansieodendaal Jun 24, 2022

I propose to add Aaron's scanning poc code here to encrypt and decrypt the value with a // TODO: note to change it to use the tari_crypto methods when implemented there.

base_layer/core/src/services/encryption_service.rs Outdated

+              }
+              #[async_trait]
+              impl ServiceInitializer for EncryptionServiceInitializer {

Contributor

hansieodendaal Jun 24, 2022

See comment above related to this not needing to be a true "service".

base_layer/core/src/services/encryption_service.rs Outdated

+              // TODO: Rename to `EncryptionFactory`
+              #[derive(Debug, Clone)]
+              pub struct EncryptionServiceHandle {}

Contributor

hansieodendaal Jun 24, 2022

As per @stringhandler this should probably not be a true "service". As can be seen from Aaron's scanning poc, the value encryption and decryption is not complex and will only do one thing.

base_layer/core/src/services/encryption_service.rs Outdated

+              // TODO: Rename to `EncryptionFactory`
+              #[derive(Debug, Clone)]
+              pub struct EncryptionServiceHandle {}

Contributor

hansieodendaal Jun 24, 2022

The encrypt and decrypt methods could probably just move back to EncryptedValue, as the only additional parameter needed will be the encryption_key if Aaron's scanning poc code is implemented directly into those methods.

therustmonk force-pushed the service_for_ecrypting_value branch 12 times, most recently from 9af4010 to 039705c Compare

June 26, 2022 06:48

therustmonk commented

View reviewed changes

comms/dht/src/crypt.rs

               use crate::{
                   envelope::{DhtMessageFlags, DhtMessageHeader, DhtMessageType, NodeDestination},
                   outbound::DhtOutboundError,
                   version::DhtProtocolVersion,
               };
-              #[derive(Debug, Clone, Zeroize, ZeroizeOnDrop)]
+              #[derive(Debug, Clone, Zeroize)]
+              #[zeroize(drop)]

Contributor Author

therustmonk Jun 26, 2022

In case if we use chacha20poly1305 crate we should use zeroize=1.4 to let cargo solve dependencies.

therustmonk force-pushed the service_for_ecrypting_value branch 8 times, most recently from 9b0f19d to 86bd89d Compare

June 27, 2022 15:04

therustmonk marked this pull request as ready for review

June 27, 2022 15:09

therustmonk force-pushed the service_for_ecrypting_value branch 2 times, most recently from 0d7de22 to 955e17f Compare

June 27, 2022 20:15

therustmonk requested a review from hansieodendaal

June 27, 2022 20:15

therustmonk requested a review from stringhandler

June 27, 2022 20:15

hansieodendaal reviewed

View reviewed changes

Contributor

hansieodendaal left a comment

Looking good apart from the the failing tests, also some comments

base_layer/wallet/src/output_manager_service/service.rs Outdated

@@ @@ -1732,10 +1757,11 @@ where @@
                       let blinding_key = PrivateKey::from_bytes(&hash_secret_key(&spending_key))?;
                       // TODO: This may be usefull for the encrypted value decryption since this is an atomic swap -
                       // TODO: when fixing 'todo_decrypt'
-                      let _rewind_key = PrivateKey::from_bytes(&hash_secret_key(&blinding_key))?;
+                      let rewind_key = PrivateKey::from_bytes(&hash_secret_key(&blinding_key))?;
+                      let encryption_key = PrivateKey::from_bytes(&hash_secret_key(&rewind_key))?;
                       // TODO: Fix this logic when 'todo_decrypt' - only commence if the tag is recognized

Contributor

hansieodendaal Jun 28, 2022

Suggested change

      
                    // TODO: Fix this logic when 'todo_decrypt' - only commence if the tag is recognized

base_layer/wallet/src/output_manager_service/service.rs Outdated

@@ @@ -1963,15 +1989,21 @@ where @@
                               )?;
                               let rewind_blinding_key = PrivateKey::from_bytes(&hash_secret_key(&spending_key))?;
                               let recovery_byte_key = PrivateKey::from_bytes(&hash_secret_key(&rewind_blinding_key))?;
+                              let encryption_key = PrivateKey::from_bytes(&hash_secret_key(&recovery_byte_key))?;
                               // TODO: Fix this logic when 'todo_decrypt' - only commence if the tag is recognized

Contributor

hansieodendaal Jun 28, 2022

Suggested change

      
                            // TODO: Fix this logic when 'todo_decrypt' - only commence if the tag is recognized

base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs Outdated

@@ @@ -97,15 +96,19 @@ where @@
                               continue;
                           }
                           // TODO: Fix this logic when 'todo_decrypt' - only commence if the tag is recognized

Contributor

hansieodendaal Jun 28, 2022

Suggested change

      
                        // TODO: Fix this logic when 'todo_decrypt' - only commence if the tag is recognized

base_layer/wallet/src/output_manager_service/service.rs Outdated

@@ @@ -1732,10 +1757,11 @@ where @@
                       let blinding_key = PrivateKey::from_bytes(&hash_secret_key(&spending_key))?;
                       // TODO: This may be usefull for the encrypted value decryption since this is an atomic swap -
                       // TODO: when fixing 'todo_decrypt'

Contributor

hansieodendaal Jun 28, 2022

Suggested change

// TODO: when fixing 'todo_decrypt'

base_layer/wallet/src/output_manager_service/service.rs Outdated

		@@ -1732,10 +1757,11 @@ where
		let blinding_key = PrivateKey::from_bytes(&hash_secret_key(&spending_key))?;
		// TODO: This may be usefull for the encrypted value decryption since this is an atomic swap -

Contributor

hansieodendaal Jun 28, 2022

Suggested change

      
                    // TODO: This may be usefull for the encrypted value decryption since this is an atomic swap -
          
                    // TODO: This may be usefull for the encrypted value decryption since this is an atomic swap

applications/test_faucet/src/main.rs Outdated

Comment on lines 203 to 204

		let encryption_keys = generate_keys();
		let encrypted_value = EncryptedValue::encrypt_value(&encryption_keys.k, &commitment, value).unwrap();

Contributor

hansieodendaal Jun 28, 2022

I think we should be using the default value here, as faucet UTXOs are not recoverable without respending them.

therustmonk force-pushed the service_for_ecrypting_value branch 3 times, most recently from 2371f85 to b4cd57a Compare

June 28, 2022 12:05

stringhandler changed the title ~~feat: service for ecrypting value~~ feat: service for encrypting value

therustmonk force-pushed the service_for_ecrypting_value branch from 09025a3 to 29695cc Compare

June 28, 2022 17:35

stringhandler reviewed

View reviewed changes

Collaborator

stringhandler left a comment

Looks good. Happy to merge if we can remove:

The FFI methods, they are not needed (and might even be a security risk)
The output manager handle methods (as far as I can see, they are only used by the FFI)

base_layer/core/src/transactions/transaction_components/encrypted_value.rs Outdated

+                      // Authenticate and decrypt the value
+                      let aead_payload = Payload {
+                          msg: value.as_bytes(),
+                          aad: b"TARI_AAD_SCAN".as_ref(),

Collaborator

stringhandler Jun 28, 2022

I think it makes sense to use VALUE here, so that in future we can add extra fields

Suggested change

      
                        aad: b"TARI_AAD_SCAN".as_ref(),
          
                        aad: b"TARI_AAD_VALUE".as_ref(),

base_layer/wallet_ffi/src/lib.rs Outdated

@@ @@ -1144,14 +1144,53 @@ pub unsafe extern "C" fn encrypted_value_as_bytes( @@
               /// memory leak
               #[no_mangle]
               pub unsafe extern "C" fn encrypted_value_encrypt(

Collaborator

stringhandler Jun 28, 2022

I don't see the need for this method? I think it should be removed from the ffi interface

Contributor

hansieodendaal Jun 29, 2022 •

edited

Loading

Yip, this one was undecided in my head as well when I added it, so agree it can be removed along with the associated output manager handle.

base_layer/wallet_ffi/src/lib.rs Outdated

@@ @@ -1168,20 +1207,61 @@ pub unsafe extern "C" fn encrypted_value_encrypt( @@
               /// memory leak
               #[no_mangle]
               pub unsafe extern "C" fn encrypted_value_decrypt(
+                  wallet: *mut TariWallet,

Collaborator

stringhandler Jun 28, 2022

Same here, this method can be used to arbitrarily decrypt some data

Contributor

hansieodendaal Jun 29, 2022

I would argue that we might keep this method. It is very handy for a wallet to be able to detect with a high degree of certainty which UTXOs it owns. That being said, a strategy of detect-ownership-then-request-all-information may very well be completely implemented in the base layer wallet and never touch the front-end.

W.r.t. "arbitrarily decrypt some data"; I think it is equally true or equally difficult for any client and not only clients using the FFI interface.

therustmonk force-pushed the service_for_ecrypting_value branch 10 times, most recently from 5c75d45 to fc0cea9 Compare

June 30, 2022 22:22

hansieodendaal approved these changes

View reviewed changes

Contributor

hansieodendaal left a comment

LGTM - all ffi integration tests and all unit tests passed locally

hansieodendaal previously approved these changes

View reviewed changes

Contributor

hansieodendaal left a comment

LGTM


          feat: add encryption service

04bc65a

therustmonk dismissed hansieodendaal’s stale review via

04bc65a

July 1, 2022 06:52

therustmonk force-pushed the service_for_ecrypting_value branch from fc0cea9 to 04bc65a Compare

July 1, 2022 06:52

stringhandler merged commit 6ce6b89 into tari-project:development

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet