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

feat: functionality to work with Hidden types #148

Merged
merged 2 commits into from Nov 11, 2022

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Nov 10, 2022

Work in tari_utilities PR 52 provides a new Hidden type for hidden data that supports safer handling of underlying data that supports the Zeroize trait. This is intended to handle things like passphrases, seed words, and derived key materials.

Initial work by @jorgeantonio21 on migrating existing key derivation function (KDF) outputs to use Hidden found two issues pretty quickly:

  • The generic SecretKey and PublicKey traits do not enforce Zeroize, even though the Ristretto-based implementations in this crate do.
  • The hashing API keeps copies of hash output data in memory that is not zeroized.

The first issue is more of an annoyance, since it means a Hidden type containing a generic SecretKey or PublicKey needs to add additional Zeroize trait bounds everywhere. This PR adds the trait bound to both. Since RistrettoSecretKey and RistrettoPublicKey already implement Zeroize, this change is seamless. This addresses part of issue 147.

The second issue is trickier. While we can't control what goes on inside the state of any underlying hash function (e.g. Blake256 in the tari crate), and while some hashing API use cases don't assume the output is sensitive, KDF use cases do. For these cases, we want to minimize non-zeroized copies of the output. This PR mitigates this by implementing FixedOutput for DomainSeparatedHasher, which adds in-place output support. The use of the required finalize_into_reset might be helpful in this case, since it doesn't consume the hasher but resets its internal state (albeit in a way that isn't clear to me).

@AaronFeickert AaronFeickert changed the title Functionality to work with Hidden types feat: functionality to work with Hidden types Nov 10, 2022
@AaronFeickert
Copy link
Contributor Author

This should be checked by @jorgeantonio21 to see if it properly addresses the issues he had.

hasher.update([0, 0, 0]);

let mut output = GenericArray::<u8, <Blake256 as Digest>::OutputSize>::default();
hasher.finalize_into(&mut output);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a new line with an assertion to compare the result of output with the same data, hashed using finalize.

hasher.update([0, 0, 0]);

let mut output = GenericArray::<u8, <Blake256 as Digest>::OutputSize>::default();
hasher.finalize_into_reset(&mut output);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@CjS77 CjS77 added P-merge The PR can me merged. and removed P-reviews_required labels Nov 11, 2022
Copy link
Contributor

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

+1

@CjS77
Copy link
Contributor

CjS77 commented Nov 11, 2022

Will merge once there's an ACK

@CjS77
Copy link
Contributor

CjS77 commented Nov 11, 2022

ACK

@CjS77 CjS77 merged commit 086d164 into tari-project:main Nov 11, 2022
@AaronFeickert AaronFeickert deleted the hidden-support branch November 11, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required P-merge The PR can me merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants