You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reviewing PR #1121: feat(prf): add support for EMS mode
Gather context and read CLAUDE.md
Read PR diff and changed files
Analyze changes and provide review feedback
Summary
This PR adds Extended Master Secret (RFC 7627) support to the PRF implementation by:
Renaming Mode → split into NetworkMode + MSMode — cleanly separates the network optimization concern (Normal vs Reduced) from the master secret derivation mode (Standard vs Extended).
Renaming MpcPrf → Prf — simpler name.
Introducing PrfConfig — bundles NetworkMode and MSMode together.
Adding set_session_hash() — new method for providing the session hash in EMS mode.
Deleting prf/state.rs — the state machine was simplified and inlined into prf.rs.
Relaxing input-setting requirements — set_client_random and set_server_random no longer enforce strict state ordering; the master secret seed is set conditionally based on MSMode.
Feedback
Overall: This is a well-structured change. The separation of NetworkMode and MSMode is clean, the EMS label/seed differences are correctly handled per RFC 7627, and tests cover all four combinations (Normal/Reduced × Standard/Extended). A few observations:
1. Potential panic in set_server_random — consider returning an error instead
prf.rs:151-153:
let client_random = self.client_random.expect("Client random should have been set by now");
Since this PR explicitly relaxes input-setting ordering requirements, the expect here is a latent panic if a caller sets the server random before the client random. Since set_server_random already returns Result<(), PrfError>, this could return a proper error instead.
This is correct — in EMS mode the session hash is the seed. But it might be worth a brief comment here explaining why the MS seed is skipped in EMS mode, since the relationship to set_session_hash() isn't immediately obvious from this code alone.
3. No enforcement that set_session_hash is called before flush in EMS mode
In set_session_hash (prf.rs:177-190), the session hash sets master_secret.set_start_seed(seed). However, if a caller forgets to call this in EMS mode, the start_seed_label will be None and flush() will panic when it hits expect("Start seed should have been set") in both normal.rs:83 and reduced.rs:106.
This isn't a regression (the Standard path has the same pattern), but since this PR is adding the EMS path, it would be a good time to consider returning errors from flush() instead of panicking on missing seeds. At minimum, a doc comment on set_session_hash noting it must be called before flush in EMS mode would help.
Both fields are pub and accessed directly (e.g., self.config.network, self.config.ms in prf.rs). Since PrfConfig already has a constructor, you might want pub(crate) visibility on the fields to keep the public API surface minimal. This is a minor style point — fine either way if direct field access from downstream is intended.
5. MSMode and NetworkMode don't derive PartialEq
config.rs:5,13: Neither enum derives PartialEq, which means the code uses matches!() for comparisons (e.g., matches!(self.config.ms, MSMode::Standard)). This works, but deriving PartialEq (and Eq) would be more idiomatic and enable == comparisons if needed in the future.
6. mpc-tls hardcodes MSMode::Standard
leader.rs:89 and follower.rs:67:
let prf = Prf::new(PrfConfig::new(config.prf,MSMode::Standard));
This is presumably intentional for this PR (EMS integration at the mpc-tls layer is future work), but worth noting that the mpc-tls Config struct only exposes NetworkMode (prf field). When EMS support is wired in, Config will need an MSMode field and the set_session_hash call path.
7. Test coverage is solid
All 4 mode combinations are tested at both the inner PrfFunction level (function.rs tests) and the outer Prf level (lib.rs tests).
A reference test against ring's PRF (test_prf_reference_ems_ms) validates correctness.
Benchmarks cover all 4 combinations.
Verdict
The implementation is correct and well-tested. The main actionable suggestion is replacing the expect in set_server_random with a proper error (point 1), since this PR explicitly aims to relax strict input ordering. The rest are minor style/documentation suggestions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR