-
Notifications
You must be signed in to change notification settings - Fork 194
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
crypto-common: add SerializableState trait #1078
Conversation
digest/src/core_api.rs
Outdated
@@ -117,3 +117,15 @@ pub enum TruncSide { | |||
/// Truncate right side, i.e. `&out[m..]`. | |||
Right, | |||
} | |||
|
|||
/// Core trait for saving internal state of the hash core and restoring it later. | |||
pub trait HashCoreInternalState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest using Rust's built-in conversion traits rather than defining special methods like internal_state
and from_internal_state
:
pub trait HashCoreInternalState { | |
pub trait HashCoreInternalState: TryFrom<Self::InternalState, Error = Error> + Into<Self::InternalState> { |
I think the core notion here is a "serializable state" rather than an "internal state" |
digest/src/core_api.rs
Outdated
/// Core trait for saving internal state of the hash core and restoring it later. | ||
pub trait HashCoreInternalState { | ||
/// Internal state of the hash core. | ||
type InternalState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to have some bounds here to enable generic code:
type InternalState; | |
type InternalState: TryFrom<&[u8]> + AsRef<[u8]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the internal state consists of several objects, is it possible to implement TryFrom<&[u8]> + AsRef<[u8]>
: e.g. internal state of SHA-2 hashes consists of the intermediate hash value (array of u32
/u64
) and the number of consumed blocks (number u64
/u128
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case of this API is serialization, and serialization needs to operate in terms of bytestrings.
If you think there's a use case beyond serialization, can you explain how that use case isn't satisfied by simply cloning an existing digest instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the use case is serialization only.
Should I then create a byte array and serialize the internal state on my own?
E.g. for Sha256VarCore
, I need to allocate an array, copy the intermediate hash value ([u32; STATE_LEN]
), and the number of consumed blocks (u64
).
Or is it better to return an object that consists of the intermediate hash value and the number of consumed blocks, and this object is Serializable
(serde
)? But this object can not implement AsRef<[u8]>
because it consists of a byte array and number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends if the state can always be fixed-width or not. If it can, we should probably use an associated generic_array::ArrayLength<u8>
instead.
I think it's a pretty reasonable assumption that most digests could support a fixed-width serializable state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's worth to introduce trait which outputs generic array. Something like this:
pub type SerializedState<T> = GenericArray<u8, <T as SerializableState>::SerializedStateSize>;
pub trait SerializableState {
type SerializedStateSize: ArrayLength<u8>;
fn serialize(&self) -> SerializedState<Self>;
fn deserialize(buf: &SerializedState<Self>) -> Self;
}
Also do not forget to add warning that serialized state may contain sensitive information, e.g. in the hashes case it can contain parts of unprocessed message.
The trait may go to the crypto-common
crate since it may be useful for other algorithms as well.
fc5c8f7
to
45e5ef5
Compare
Sorry, missed the last comment. I will move the trait to Should |
The versions of the crates have to be updated: should they be Right now crypto-common is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should deserialize method return Result (if the buffer contains invalid values)?
Yes, I forgot to do it in my snippet. I think we can define the error type in crypto-common
(i.e. no need to make it generic).
The versions of the crates have to be updated: should they be 0.1.7/0.10.4 or 0.2.0/0.11.0?
The changes are backwards-compatible, so patch releases should be fine.
There is a small issue with It is needed for
Is there any known Or should it be implemented via
What should be the best place to put it In this case? |
The serialized state needs to be consistent regardless of the target pointer width. You should pick a sensible on-the-wire representation ( |
I think we can simply transform |
digest/src/core_api/ct_variable.rs
Outdated
|
||
fn serialize(&self) -> SerializedState<Self> { | ||
self.inner.serialize() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a sanity check it could be worth to add into serialized state output size (the OutSize
type parameter) and return an error during deserialization if it does not match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use the concat
method to slightly improve the code, though it could force you to make SerializedStateSize
less... pretty. So it's quite optional.
digest/src/core_api/rt_variable.rs
Outdated
<T::SerializedStateSize as Add<U9>>::Output: Add<T::BlockSize>, | ||
<<T::SerializedStateSize as Add<U9>>::Output as Add<T::BlockSize>>::Output: ArrayLength<u8>, | ||
{ | ||
type SerializedStateSize = Sum<Sum<T::SerializedStateSize, U9>, T::BlockSize>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you forgot to change U9
to U2
.
digest/src/core_api/rt_variable.rs
Outdated
|
||
Ok(Self { | ||
core: T::deserialize(serialized_core)?, | ||
buffer: BlockBuffer::new(&block_buffer[..pos]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if the buffer is "eager" and pos
is equal to block size, new
will panic. I guess the simplest solution will be to add try_new
method to BlockBuffer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
digest/src/core_api/wrapper.rs
Outdated
|
||
Ok(Self { | ||
core: T::deserialize(serialized_core)?, | ||
buffer: BlockBuffer::new(block_buffer), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in the previous comment.
Converted to draft to avoid emails about failed tests. UPD: Did not help :( |
793f9f2
to
bc9dd77
Compare
With the introduction of the |
No, those are type-level properties which are identical regardless of the state. |
What do you think about 3447449? |
3611c22
to
2ad3d70
Compare
Could you please have a look at afc029e ? This is the implementation of The current implementation doesn't support generics for now. Also, |
👋 any chance of getting this merged any time in the near future? |
@waynr it needs a rebase, at the very least |
@tarcieri I'm attempting to rebase but running into compile errors that are pretty much totally incomprehensible to me. This repo is like a firehose of generic types and my puny little brain only has capacity for a trickle. |
I think @rpiasetskyi said he would take a look |
I figured out that the problem had to do with updating the |
Just a quick note - it looks like the issue of the local dependency on crypto-common being broken in master branch for the digest crate is fixed in #1358 so this PR might need to wait for that before being rebased. |
I am doing a rebase (and migration to |
@tarcieri Sorry, I was expecting it to update the PR but not to merge it :( |
Reopen. |
I think you pushed 0 commits which auto-closed the PR? Regardless I can't reopen it... I think you'd need to push some new commits |
This trait is used for saving internal state of the hash core and
restoring it later.
The original issue was discussed here: RustCrypto/hashes#310