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(wallet)!: use KDFs on ECDH shared secrets #4847

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Oct 24, 2022

Description

Uses KDFs on ECDH shared secrets for output generation. Closes issue 4717.

Motivation and Context

Several uses of ECDH shared secrets in the output manager and transaction services parse an ECDH shared secret as a scalar spending key, and use this as input to a chain of hash functions for use in rewinding and value encryption. This is non-standard.

This work uses separate KDFs to independently produce a spending key, rewind key, and value encryption key from a DiffieHellmanSharedSecret-type ECDH shared secret.

How Has This Been Tested?

Existing tests pass.

BREAKING CHANGE: Changes the way output keys are derived.

@AaronFeickert AaronFeickert changed the title fix(wallet): use KDFs on ECDH shared secrets WIP: fix(wallet): use KDFs on ECDH shared secrets Oct 24, 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 24, 2022
@hansieodendaal
Copy link
Contributor

I like this approach, also getting rid of repetitive key hashing.

@stringhandler stringhandler changed the title WIP: fix(wallet): use KDFs on ECDH shared secrets WIP: fix(wallet)!: use KDFs on ECDH shared secrets Oct 27, 2022
@stringhandler
Copy link
Collaborator

Thanks. This looks like it is a breaking change. Please add a BREAKING CHANGE: footer to the PR description

@AaronFeickert AaronFeickert changed the title WIP: fix(wallet)!: use KDFs on ECDH shared secrets fix(wallet)!: use KDFs on ECDH shared secrets Nov 2, 2022
@AaronFeickert AaronFeickert marked this pull request as draft November 2, 2022 16:46
@stringhandler stringhandler added W-transaction-breaking Warning - Changes data that wallets use to send transactions A-wallet Area - related to the wallet labels Nov 7, 2022
@AaronFeickert AaronFeickert force-pushed the fix-output-scanning-kdfs branch from b506a3d to d48f3ae Compare November 7, 2022 22:21
@AaronFeickert AaronFeickert marked this pull request as ready for review November 7, 2022 22:21
@stringhandler stringhandler added the P-merge Process - Queued for merging label Nov 8, 2022
@stringhandler
Copy link
Collaborator

utACK

@stringhandler stringhandler merged commit 3d1a51c into tari-project:development Nov 8, 2022
@AaronFeickert AaronFeickert deleted the fix-output-scanning-kdfs branch November 8, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-wallet Area - related to the wallet P-acks_required Process - Requires more ACKs or utACKs P-merge Process - Queued for merging P-reviews_required Process - Requires a review from a lead maintainer to be merged W-transaction-breaking Warning - Changes data that wallets use to send transactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diffie-Hellman results are parsed as scalars during output recovery
4 participants