Skip to content

digest: add FixedOutputDirty trait + finalize_into* - #180

Merged
tarcieri merged 1 commit into
masterfrom
digest/finalize-fixed-into
Jun 9, 2020
Merged

digest: add FixedOutputDirty trait + finalize_into*#180
tarcieri merged 1 commit into
masterfrom
digest/finalize-fixed-into

Conversation

@tarcieri

@tarcieritarcieri commented Jun 9, 2020

Copy link
Copy Markdown
Member

ClosesRustCrypto/hashes#86

Replaces the Reset trait with a set of _reset methods on all of the various traits, and uses these as the default impl for the methods which consume hashers.

Also adds a set of methods to FixedOutput (finalize_fixed_into*) which eliminate copies by accepting an existing buffer as an argument.

Adds a FixedOutputDirty trait which writes the digest output to a provided byte array, but does not reset the internal state. This is intended for implementations to use in order to ensure that they are not reset in the event the instance is consumed.

Also adds a set of finalize_into and finalize_into_reset methods to FixedOutput whhich also write their input into a provided byte array, and changes the existing finalize_fixed (and newly added finalize_fixed_reset) methods to have a default implementation which returns a byte array allocated on the stack.

Finally, adds a blanket impl of FixedOutput for FixedOutputDirty + Reset types which handles safely invoking the underlying implementation by either consuming the instance (avoiding a reset) or borrowing the hasher, obtaining the output, and resetting.

@tarcieri
tarcieri requested a review from newpavlovJune 9, 2020 14:51
@tarcieri

tarcieri commented Jun 9, 2020

Copy link
Copy Markdown
MemberAuthor

@newpavlov this is slightly different from what you originally proposed, but I think achieves the same effect. Namely if you wanted something exactly like what you had before, it could be achieved with:

implMyDigest{/// Private method not intended to be used directlyfnfinalize_fixed_core(&mutself,out:&mutGenericArray<u8,Self::OutputSize>,reset:bool){[...]}}implFixedOutputforMyDigest{typeOutputSize = [...];#[inline]fnfinalize_fixed(self) -> GenericArray<u8,Self::OutputSize>{letmut buf = Default::default();self.finalize_fixed_core(&mut buf,false);
buf
}#[inline]fnfinalize_fixed_reset(&mutself) -> GenericArray<u8,Self::OutputSize>{ .. }fnfinalize_fixed_into(self,out:&mutGenericArray<u8,Self::OutputSize>){ .. }fnfinalize_fixed_into_reset(&mutself,out:&mutGenericArray<u8,Self::OutputSize>){ .. }}

...it's just by not making it part of the trait, it could actually be a private method.

Comment threaddigest/src/dyn_digest.rs Outdated
@tarcieri
tarcieriforce-pushed the digest/finalize-fixed-into branch from 47d5f10 to a2f119eCompareJune 9, 2020 15:02

@newpavlovnewpavlov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One problem with this approach is that it makes harder for compiler to eliminate reset code, this is why I was thinking about using the private method, which would keep reset code behind a simple branch which can eliminated by trivial const propagation. Before going with this approach, it would be nice to see that compiler is indeed will be able to properly remove reset code for finalize_fixed.

@tarcieri

Copy link
Copy Markdown
MemberAuthor

That's only if you rely on the default implementation though. If you want to avoid the resets entirely, just define your own implementation (per above) which doesn't invoke the reset code at all.

@newpavlov

newpavlov commented Jun 9, 2020

Copy link
Copy Markdown
Member

This will lead to unnecessary code duplication, which I would like to avoid.

How about something like this?

pubtraitFixedOutput{// .../// Retrieve result into provided buffer and leave hasher in a dirty state./// Usage of this method in user code is discouraged, prefer `finalize_fixed`/// or `finalize_fixed_reset`.fnfinalize_into_dirty(&mutself,out:&mutGenericArray<u8,Self::OutputSize>);fnfinalize_into_reset(&mutself,out:&mutGenericArray<u8,Self::OutputSize>)whereSelf:Reset,{self.finalize_into_dirty(out);self.reset();}}pubtraitReset{fnreset(&mutself);}

Yes, users may misuse finalize_into_dirty (use a scarier name?), but I don't think it will be a significant issue in practice.

@tarcieri

tarcieri commented Jun 9, 2020

Copy link
Copy Markdown
MemberAuthor

I think you could achieve something like that with a separate trait and a blanket impl:

pubtraitFixedOutputDirty{/// Retrieve result into provided buffer and leave hasher in a dirty state./// Usage of this method in user code is discouraged, prefer `finalize_fixed`/// or `finalize_fixed_reset`.fnfinalize_into_dirty(&mutself,out:&mutGenericArray<u8,Self::OutputSize>);}impl<D:FixedOutputDirty + Reset>FixedOutputforD{#[inline]fnfinalize_into(&mutself,out:&mutGenericArray<u8,Self::OutputSize>)whereSelf:Reset,{self.finalize_into_dirty(out);}#[inline]fnfinalize_into_reset(&mutself,out:&mutGenericArray<u8,Self::OutputSize>)whereSelf:Reset,{self.finalize_into_dirty(out);self.reset();}}

@tarcieri

Copy link
Copy Markdown
MemberAuthor

Also should I shorten finalize_fixed_into to finalize_into? Seems ok to me.

@newpavlov

Copy link
Copy Markdown
Member

The separate trait looks good, making room for FixedOutput to be used separately from the dirty trait (e.g. for HW accelerated impls) is certainly nice.

Also should I shorten finalize_fixed_into to finalize_into?

Yes, sounds good.

@tarcieri
tarcieriforce-pushed the digest/finalize-fixed-into branch from a2f119e to 7ef7beeCompareJune 9, 2020 17:14
@tarcieritarcieri changed the title digest: replace Reset trait with methods; add *into* methodsdigest: add FixedOutputDirty trait + finalize_into*Jun 9, 2020
@tarcieri
tarcieriforce-pushed the digest/finalize-fixed-into branch from 7ef7bee to daf6cfbCompareJune 9, 2020 17:17
@tarcieri

tarcieri commented Jun 9, 2020

Copy link
Copy Markdown
MemberAuthor

Updated to use a FixedOutputDirty trait + blanket impl. I like this approach especially because it's a much less invasive change.

@tarcieri
tarcieriforce-pushed the digest/finalize-fixed-into branch from daf6cfb to 4f53f96CompareJune 9, 2020 17:19
Adds a `FixedOutputDirty` trait which writes the digest output to a
provided byte array, but does not reset the internal state. This is
intended for implementations to use in order to ensure that they are
not reset in the event the instance is consumed.
Also adds a set of `finalize_into` and `finalize_into_reset` methods to
`FixedOutput` whhich also write their input into a provided byte array,
and changes the existing `finalize_fixed` (and newly added
`finalize_fixed_reset`) methods to have a default implementation which
returns a byte array allocated on the stack.
Finally, adds a blanket impl of `FixedOutput` for `FixedOutputDirty` +
`Reset` types which handles safely invoking the underlying
implementation by either consuming the instance (avoiding a reset)
or borrowing the hasher, obtaining the output, and resetting.
@tarcieri
tarcieriforce-pushed the digest/finalize-fixed-into branch from 4f53f96 to 2526a6aCompareJune 9, 2020 17:24
@tarcieri
tarcieri merged commit 496aa73 into masterJun 9, 2020
@tarcieri
tarcieri deleted the digest/finalize-fixed-into branch June 9, 2020 17:30
@newpavlov

Copy link
Copy Markdown
Member

BTW should we do something similar for XOF and VariableOutput?

@tarcieri

Copy link
Copy Markdown
MemberAuthor

Sure

tarcieri added a commit that referenced this pull request Jun 9, 2020
Adds traits which provide an implementation-focused low-level API
similar to the `FixedOutputDirty` trait introduced in #180,
but for the `ExtendableOutput` and `VariableOutput` traits.
Like `FixedOutputDirty`, these traits have blanket impls when the same
type also impls the `Reset` trait.
Also adds corresponding `*_reset` methods to the respective `finalize*`
methods of these traits.
tarcieri added a commit that referenced this pull request Jun 9, 2020
…183)
Adds traits which provide an implementation-focused low-level API
similar to the `FixedOutputDirty` trait introduced in #180,
but for the `ExtendableOutput` and `VariableOutput` traits.
Like `FixedOutputDirty`, these traits have blanket impls when the same
type also impls the `Reset` trait.
Also adds corresponding `*_reset` methods to the respective `finalize*`
methods of these traits.
@tarcieritarcieri mentioned this pull request Jun 10, 2020
dns2utf8 pushed a commit to dns2utf8/traits that referenced this pull request Jan 24, 2023
Adds a proper conversion between argon2::Error and password_hash::Error
and uses it when hashing the password.
Sign up for freeto 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.

Add fixed_result_into to prevent copy

2 participants

@tarcieri@newpavlov