Skip to content

New feature: Suspendable (serializable state)#48

Merged
ounsworth merged 20 commits into
bcgit:release/0.1.2alphafrom
ounsworth:feature/serialize_state
Jul 10, 2026
Merged

New feature: Suspendable (serializable state)#48
ounsworth merged 20 commits into
bcgit:release/0.1.2alphafrom
ounsworth:feature/serialize_state

Conversation

@ounsworth

@ounsworth ounsworth commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Closes #24

This was a feature requested by Simo to be able to pause an operation -- particularly in the middle of a do_update() ... do_update() ... do_final() flow -- stick the serialized crypto operation into a cache, and then resume it later. This is an important feature for implementing TLS loadbalancers that want to be able to cache out the HKDF state and resume it from a different box when the next flight of TCP messages come in.

The only "creative" thing I've added here is that all serialized states are prefixed with a 3-byte semantic version of the library and it's checked on load back in. The idea is that if we ever need to change the serialization of some object, then we can add a guard to refuse to load a serialized object from before the guard (or keep both versions of the deserializer and switch based on the version tag, or whatever ... #futureProofing).

@ounsworth

ounsworth commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

For now, I have added the trait and impl'd it for SHA2.

I'll pause here and give a chance for people to give feedback before I continue with doing the same for:

  • SHA3
  • HMAC
  • HKDF
  • ML-DSA
  • ML-DSA_lowmemory

Comment thread crypto/core/src/serializable_state.rs Outdated
Comment thread crypto/sha2/src/sha256.rs
// insert the version tag
let out: &mut [u8; 105] = add_lib_ver(&mut out_to_return).try_into().unwrap();

// state.h: [u32; 8]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is state.h?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The internal state of SHA2. It is called H^i_j in FIPS 180-4, see sections 2.2 and 6.2.

Comment thread crypto/sha2/src/sha256.rs
Comment thread crypto/sha2/src/sha256.rs Outdated

impl<PARAMS: SHA2Params> SerializableState<108> for SHA256Internal<PARAMS> {
fn serialize_state(&self) -> [u8; 108] {
let mut out_to_return = [0u8; 108];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where 108 and other sizes coming from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have replaced all the magic numbers with named pub const's to make it clearer.

Comment thread crypto/sha2/Cargo.toml Outdated
@dghgit

dghgit commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

It looks okay, it'll really help with SLH-DSA as storing intermediate state is almost a requirement to getting the algorithm to work in real time... two things, and you may have to pardon my rust if I've got this wrong. It looks like the version check is automatically rejecting anything older, shouldn't that be newer? Ideally you'd be compatible 1 or 2 versions older, newer might be a bit problematic as you might not know the format. Also, for what it's worth, the LLM suggested LIB_VERSION could be derived from env!("CARGO_PKG_VERSION") with the actual version been something taken from the core API crate.

@ounsworth

Copy link
Copy Markdown
Contributor Author

It looks like the version check is automatically rejecting anything older, shouldn't that be newer?

My idea was to have a framework to be able to carry multiple versions of the parser in case we ... I don't know ... add a boolean flag to one of the objects for some reason.

I honestly hadn't thought about blocking future versions. That's a good idea!

Also, for what it's worth, the LLM suggested LIB_VERSION could be derived from env!("CARGO_PKG_VERSION").

I also did some research but didn't find anything useful. Let me give this a bit more poking. Thanks David!

@ounsworth ounsworth force-pushed the feature/serialize_state branch from 2386f22 to 54f93f6 Compare July 6, 2026 04:00
@ounsworth

Copy link
Copy Markdown
Contributor Author

I don't know why 2d0b7f3 got included in this PR. I also don't know how to remove it. Sorry.

@ounsworth ounsworth force-pushed the feature/serialize_state branch from 46df1d1 to 937d859 Compare July 8, 2026 01:45
@ounsworth ounsworth marked this pull request as ready for review July 8, 2026 02:33
@ounsworth ounsworth changed the title New feature: SerializableState New feature: Suspendable (serializable state) Jul 8, 2026
Comment thread crypto/hkdf/src/lib.rs Outdated
// If ever we need to version-reject HKDF separately from HMAC, then we'll need to add
// an explicit version check here, and change "None" to the oldest accepted version.
// _ = check_lib_ver(&serialized_state, None)?;
let hmac = match state[$serialized_hmac_len] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

from_suspended doesn't cross-check the hmac-present flag against the HkdfStates tag. A blob with the present-flag = 0 but state = Initialized/TakingAdditionalInfo deserializes fine and then panics on self.hmac.unwrap() in do_extract_final/do_extract_update_*. This panic is a DoS.

Per the trait contract, this should be rejected with InvalidData. Suggest adding if (hkdf_state == HkdfStates::Uninitialized) != hmac.is_none() { return Err(SerializedStateError::InvalidData) } and a regression test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


/// The current library version, taken from this crate's `Cargo.toml` at compile time (via Cargo's
/// `CARGO_PKG_VERSION_*` env vars) so it can never drift from the published version.
pub const LIB_VERSION: SemVer = SemVer {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LIB_VERSION comes from core's Cargo.toml, which is 0.1.1 here (release is 0.1.2alpha).

Stamp doesn't match the release. As the only compat (compatbility) gate, a format-breaking change in a primitive crate (example: sha2, sha3, hkdf, etc.) wouldn't bump core, hence NOT being rejected.

The old blob would be misplaced. Can we reconcile versions and add a maintainer note that any layout change must bump core's version + set not_before?

@ounsworth ounsworth Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we certainly have to make sure all the cargo files are bumped to "0.1.2", but that's unrelated to this PR. I'll go do that on the release branch.

Comment thread crypto/hkdf/src/lib.rs Outdated
// If ever we need to version-reject HKDF separately from HMAC, then we'll need to add
// an explicit version check here, and change "None" to the oldest accepted version.
// _ = check_lib_ver(&serialized_state, None)?;
let hmac = match state[$serialized_hmac_len] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

state[$serialized_hmac_len] == 0

The inner HMAC is never parsed, so the version header is never checked. Maybe we should uncomment the check_lib_ver(&state, None)? at the top so version gating holds even when hmac is absent

@ounsworth ounsworth Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This comment is a bit confusing.
I think you mean that HMAC might be absent (since it's optional), in which case it might not be parsed, not that it is never parsed?

But fair point, the possibility of an absent HMAC is a pretty good reason to add those extra 3 bytes so that this always carries a lib_ver tag, independently of the HMAC blob.

Fixed in 3c8d64b

Comment thread crypto/sha3/src/keccak.rs Outdated
if bits_in_queue > rate {
return Err(SerializedStateError::InvalidData);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

from_serialized_state doesn't fully validate bits_in_queue. A blob with squeezing = false and an odd bits_in_queue (or == rate) deserializes OK, then panics on the next absorb/pad_and_switch ("attempt to absorb with odd length queue" / the debug_assert).

Suggest rejecting those here with InvalidData, so tampered states fail cleanly. Corrupt input should be made as an InvalidData, not a panic. Also dangerous since it has a broader blast radius, as SHA3/SHAKE and ML-DSA (via MuBuilder) suspend depends on it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one might be better for you to add the guard.
I could probably do it based on this description (or I could feed your description into claude), but if you know exactly which conditions need to to be tested for, maybe better if you do it?

Comment thread crypto/hmac/src/lib.rs
/// Length in bytes of the serialized state of [HMAC_SHA3_512].
pub const SUSPENDED_HMAC_SHA3_512_STATE_LEN: usize = SUSPENDED_SHA3_STATE_LEN;

/// HMAC is a keyed algorithm, so it implements [SuspendableKeyed] (rather than

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should add a security considerations note here: the HMAC/HKDF suspended blob is the inner-hash state over K ⊕ ipad / the secret IKM: specifically, it is derived from secret material. It can't be used to forge a tag without the key, but it does commit to the key and should be stored as confidentially as the key.

The current wording ("the key is NOT included") reads as "the blob is safe to cache anywhere," which I don't think we want.

Minor note: Separately: HMAC holds key: [u8; N] but impls neither Secret nor Drop. Preexisted before this PR,, but relevant.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also thought about adding a security consideration on this point, and decided against it.
My reasoning is this:
I assume it's sorta implied that serializing the internal state of a crypto object ... well, exports the internal state of a crypto object, so you should handle it accordingly. I guess I could add a note to that effect to the top-level Suspendable trait.

In this particular case. You cannot extract the long-term key from the state because that would require you to break the hash function.
As you note, you also can't finish the HMAC / HKDF without the proper key being re-supplied.
So, it's not what "bad thing" an attacker could do if they get their hands on an HMAC / HKDF state, so I would think you actually could cache these on your public github and nothing bad would happen.

(I try to keep security consideration notes to a minimum because if you start putting low-value ones on everything then the really important ones won't jump out as much)

@ounsworth ounsworth Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Minor note: Separately: HMAC holds key: [u8; N] but impls neither Secret nor Drop. Preexisted before this PR,, but relevant.)

That's actually a pretty significant omission! I'll fix that.

Fixed in 07889fc.
That ended up being a more involved fix than I expected because I ran into some rust compiler business when I added Drop to the HMAC struct. Had to go learn how to use core::mem::take. New skill acquired!

Comment thread crypto/core/src/traits.rs
Comment thread crypto/hkdf/src/lib.rs
Comment thread crypto/core/src/errors.rs
Comment on lines +80 to +84
InvalidData,
/// The key supplied to [crate::traits::SuspendableKeyed::from_suspended] does not
/// match the key the state was created with (it is bound to a different public-key hash `tr`).
IncorrectKey,
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IncorrectKey is never constructed, and its doc claims tr-based wrong-key detection that isn't implemented (MuBuilder::from_suspended takes no key; HMAC/HKDF can't detect it).

Recommend either wire up the detection or remove the variant ,so callers don't assume wrong-key protection that doesn't exist.

@ounsworth ounsworth Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I added that when I was impl'ing Suspendable for ML-DSA, but ended up backing that out and only impl'ing it for ML-DSA::MuBuilder. So that was a leftover.
Now removed.

Comment thread crypto/sha2/src/lib.rs
Comment on lines +91 to +92
pub type SHA384 = SHA512Internal<SHA384Params>;
pub type SHA512 = SHA512Internal<SHA512Params>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Source-breaking changes to public items:

Sha512Internal -> SHA512Internal

Recommend a "breaking changes" entry to release notes here

@ounsworth ounsworth Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The whole library is currently declared ALPHA for exactly this reason: we're still actively changing it and every commit is making braking changes. There's even a big warning box at the top of the main readme saying as much.

We'll start paying more attention to this sort of thing once we declare a 1.0.0 release.

/// Input: Public key 𝑝𝑘 ∈ 𝔹32+32𝑘(bitlen (𝑞−1)−𝑑) and message 𝑀′ ∈ {0, 1}∗ .
/// Input: Signature 𝜎 ∈ 𝔹𝜆/4+ℓ⋅32⋅(1+bitlen (𝛾1−1))+𝜔+𝑘.
fn verify_mu_internal(pk: &PK, mu: &[u8; 64], sig: &[u8; SIG_LEN]) -> bool {
fn verify_mu(pk: &PK, mu: &[u8; 64], sig: &[u8; SIG_LEN]) -> Result<(), SignatureError> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Another public breaking change:

verify_mu_internal -> bool to verify_mu -> Result object

Fine for an alpha, but please add "Breaking Changes" entry to the release notes

@ounsworth ounsworth Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The entirety of ML-DSA is a new feature since the 0.1.1 release. You can't have a breaking change in a feature that didn't exist before.

Also, we're still in ALPHA exactly for this reason: so that we can just build the library without worrying about breaking changes.

state: &[u8; SERIALIZED_LEN],
not_before: Option<[u8; 3]>,
) -> Result<&[u8], SerializedStateError> {
let ver_bytes: [u8; 3] = state[..3].try_into().unwrap();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(Nit) Per QUALITY_AND_STYLE, each .unwrap() needs a justifying comment. These slice→array unwraps are infallible by const sizing

Recommend adding a short // infallible: ... comment on each and confirm quality_stats.sh fallibility didn't regress?

@ounsworth ounsworth Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, that points out a potential bug: we should check that state is at least 3 bytes long. (in practice this won't be testable from the outside since each object that uses this will static-size their array, but it could use a guard here).
Fixed.

Comment thread crypto/core/src/serializable_state.rs Outdated
};

// Also not compatible with future versions.
if ver > LIB_VERSION {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(Nit) A state produced by a newer build (even a patch bump) is rejected by an older build. One sentence to specify: states are backward-compatible only (newer lib can read older blob down to not_before; older lib rejects newer blob).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This comment got me thinking. Maybe it should accept anything from a newer patch stream as itself? It's literally the definition of PATCH that you don't make breaking changes.
https://semver.org/

I'll adjust the docstring 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 07889fc

@officialfrancismendoza

Copy link
Copy Markdown

Added comments. Ranges from nits, to missing tests + guards that should be added. Additional tests should include:

  • HMAC suspend/resume with an over-length key (>block): the round-trip test uses a 32-byte key, so the pre-hash branch of load_key_material inside from_suspended is never exercised on the resume path. Recommend adding a 200-byte key case.
  • SHAKE suspend/resume mid-squeeze: current tests only suspend during absorb. Suspend after squeezing some output, resume, assert the remaining output matches.
  • HMAC wrong-key resume (characterization): assert that resuming with a different key produces a different tag — locks in the documented "silent" behavior so it can't regress to something worse (e.g. a panic) unnoticed.

…ft as a TODO, left version number bumping as a TODO, passed bcgit#6 wycheproof randomized case, suspend and serialize sha2/sha3/shake regressions (bcgit#48)
@ounsworth ounsworth force-pushed the feature/serialize_state branch from 7f1e1ce to 751c033 Compare July 10, 2026 21:27
@ounsworth

Copy link
Copy Markdown
Contributor Author

Merged

@ounsworth ounsworth merged commit ad7dbc2 into bcgit:release/0.1.2alpha Jul 10, 2026
5 checks passed
@ounsworth ounsworth deleted the feature/serialize_state branch July 10, 2026 21:33
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.

4 participants