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: add checking for Blake256 parameters #117

Merged

Conversation

therustmonk
Copy link
Contributor

It adds checking for Blake256 parameters: salt and persona should be less or equal 16 bytes length.

Motivation: the fixes for #110.

@therustmonk therustmonk force-pushed the dk-blake2-with-params-result branch 2 times, most recently from a6ff213 to fa12759 Compare July 11, 2022 11:01
hansieodendaal
hansieodendaal previously approved these changes Jul 11, 2022
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM

pub enum HashError {
/// Length of input data exceeded a limit
#[error("wrong length")]
WorngLength,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WorngLength,
WrongLength,

))
pub fn with_params(key: &[u8], salt: &[u8], persona: &[u8]) -> Result<Self, HashError> {
if salt.len() > 16 || persona.len() > 16 {
Err(HashError::WorngLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Err(HashError::WorngLength)
Err(HashError::WrongLength)

@hansieodendaal
Copy link
Contributor

hansieodendaal commented Jul 12, 2022

I propose we add another function to make full use of the flexibility provided by VarBlake2b::with_params

pub fn with_params_var_size(key: &[u8], salt: &[u8], persona: &[u8], output_size: usize) -> Result<Self, HashError>

}

/// Constructs a `Blake256` hashing context with an explicitly specified output size.
pub fn with_params_var_size(
Copy link
Contributor

@hansieodendaal hansieodendaal Jul 12, 2022

Choose a reason for hiding this comment

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

I think we need a unit test for this and for pub fn with_params testing all the edge cases.

@stringhandler stringhandler merged commit d23fbbc into tari-project:main Jul 26, 2022
aviator-app bot pushed a commit to tari-project/tari that referenced this pull request Aug 8, 2022
Description
---
The update for the upcoming `tari-crypto` updates:
- tari-project/tari-crypto#117
- tari-project/tari-crypto#112

Motivation and Context
---


How Has This Been Tested?
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants