Skip to content
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.

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

fix(dht): use new DHKE shared secret type #4844

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Oct 21, 2022

Description

Ensures safer use of ECDH shared secrets by switching to the new DiffieHellmanSharedSecret type. Updates tari-crypto to v0.15.7 to accomplish this.

Motivation and Context

Currently, an ECDH secret used for message keys is produced as a RistrettoPublicKey, converted to bytes, and returned as a byte array. However, neither the RistrettoPublicKey nor the byte array are cleared when dropped. In conjunction with tari-crypto PR 137, this work ensures both the RistrettoPublicKey and byte array representations of the ECDH secret are zeroized on drop by using that PR's new DiffieHellmanSharedSecret type.

How Has This Been Tested?

Tested after applying tari-crypto PR 137, which adds the new DiffieHellmanSharedSecret generic type.

@AaronFeickert AaronFeickert changed the title fix[dht]: zeroize ECDH secret fix(dht): zeroize ECDH secret Oct 21, 2022
@CjS77 CjS77 added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 21, 2022
comms/dht/src/crypt.rs Outdated Show resolved Hide resolved
sdbondi
sdbondi previously approved these changes Oct 24, 2022
@CjS77 CjS77 removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 24, 2022
@sdbondi
Copy link
Member

sdbondi commented Oct 24, 2022

ACK

@CjS77 CjS77 removed the P-acks_required Process - Requires more ACKs or utACKs label Oct 24, 2022
@AaronFeickert AaronFeickert changed the title fix(dht): zeroize ECDH secret fix(dht): use new DHKE shared secret type Oct 24, 2022
@CjS77 CjS77 added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 24, 2022
@stringhandler stringhandler added the P-merge Process - Queued for merging label Oct 25, 2022
stringhandler
stringhandler previously approved these changes Oct 25, 2022
@CjS77 CjS77 removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 25, 2022
@hansieodendaal
Copy link
Contributor

utACK

@CjS77 CjS77 added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 27, 2022
@AaronFeickert
Copy link
Collaborator Author

Now updates tari-crypto to v0.15.7 and is safe to merge once CI passes.

@AaronFeickert AaronFeickert force-pushed the zeroize-shared-secrets branch 2 times, most recently from 50eeff5 to 667757e Compare October 27, 2022 22:12
@AaronFeickert
Copy link
Collaborator Author

This is currently blocked on tari-crypto issue 142, which addresses the underlying CI failure.

@sdbondi
Copy link
Member

sdbondi commented Oct 28, 2022

@AaronFeickert Try adding the tari_crypto/wasm feature

base_layer/key_manager/Cargo.toml

- wasm = ["wasm-bindgen", "js", "tari_common_types", "console_error_panic_hook"]
+ wasm = ["tari_crypto/wasm", "wasm-bindgen", "js", "tari_common_types", "console_error_panic_hook"]

@stringhandler stringhandler added the W-network-breaking Warn - Contains changes that will not work with existing nodes on a network level label Nov 7, 2022
@stringhandler
Copy link
Collaborator

I had confused this with another issue. This is not network breaking

@stringhandler stringhandler removed the W-network-breaking Warn - Contains changes that will not work with existing nodes on a network level label Nov 7, 2022
@CjS77 CjS77 removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 7, 2022
@stringhandler stringhandler merged commit 234571d into tari-project:development Nov 7, 2022
@AaronFeickert AaronFeickert deleted the zeroize-shared-secrets branch November 7, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-merge Process - Queued for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants