Refactoring to support future methods by ElsaLopez133 · Pull Request #406 · lake-rs/lakers

ElsaLopez133 · 2026-02-23T10:54:04Z

Refactoring current code to add additional methods in the future (PSK)

Added method-specific states (ProcessingM2StatStat, ProcessingM3StatStat), that diverge from PSK, to avoid using Option<> variables.
Included method in some states to be able to match on it.
Included helper functions to avoid Hax errors (DirectAndMut) (r_verify_message_3_statstat, i_verify_message_2_statsat)

…rors

For each function we match on state and create method-specific fiunctions. For example, we have r_prepare_message_2 and we match based on emthod to either use t_prepare_message_2_statstat or r_prepare_message_2_psk

WilliamTakeshi

Everything looks great to me.

Just a suggestion for the next PR (that you probably know). create a file for StatStat and one for PSK to organize it better the functions foo_statstat and foo_psk

Approved 🚀

lib/src/edhoc.rs

chrysn · 2026-02-23T20:32:14Z

shared/src/lib.rs

+#[derive(Debug)]
+pub enum ProcessingM2 {
+    StatStat(ProcessingM2StatStat), // PSK(PProcessingM2PSK) // To be added later
+}
 #[derive(Debug)]
 #[repr(C)]
-pub struct ProcessingM2 {
+pub struct ProcessingM2StatStat {
    pub mac_2: BytesMac2,
    pub prk_2e: BytesHashLen,
    pub th_2: BytesHashLen,
    pub x: BytesP256ElemLen,
    pub g_y: BytesP256ElemLen,
    pub plaintext_2: BufferPlaintext2,
    pub c_r: ConnId,
    pub id_cred_r: IdCred,
    pub ead_2: EadItems,
 }


This makes all the state of ProcessingM2 & co dependent on the method, rather than (as in the last call) just the parts that differ in other methods. Which of those fields are actually different in PSK?

I'd have expected something more like

#[derive(Debug)] pub struct ProcessingM2 { pub method_specifics: ProcessingM2MethodSpecifics, pub prk_2e: BytesHashLen, pub th_2: BytesHashLen, pub x: BytesP256ElemLen, pub g_y: BytesP256ElemLen, pub plaintext_2: BufferPlaintext2, pub c_r: ConnId, pub ead_2: EadItems, } pub enum ProcessingM2MethodSpecifics { StatStat { pub mac_2: BytesMac2, pub id_cred_r: IdCred, }, // PSK, -- is empty, but in other stages it might have fields that StatStat has not. }

so that the 90% of the code that would be duplicated can still reach for the common items.

We avoid repetition of shared-values among methods This affects ProcessingM2 and ProcessingM3

shared/src/lib.rs

Adding enum in prepare_message_2 to define inputs based on method

- Made static keys I and R method dependednt (not needed ofr PSK) - Added high level identity enums - Made i_verify_message_2 take optional I - Added ProcessedM2MethodSpecifics and ParsedMessage2Detials - Updated i_parse_message_2 and high level initiaotr API - Adapted lakers-python and lakers-c

chrysn

(I'm a bit sloppy in the review a bit on the C side, treating that as "if the test pass it will be fine", but I don't treat that as a high-priority feature.)

Doing the changes (including back-and-forth on how much is in the structs) and file-to-file moves makes this rather hard to review. Nothing sensible to be done here though -- these kinds of refactor are known to be hard, and there are no good generic solutions. (Moving things into different files only after all other changes are understood to be good would have helped, but that's hard to roll back again w/o starting over).

Not for other reviewers: --ignore-all-space is your friend.

chrysn · 2026-03-05T07:52:50Z

shared/src/lib.rs

    pub plaintext_2: BufferPlaintext2,
    pub c_r: ConnId,
    pub id_cred_r: IdCred,
    pub ead_2: EadItems,


Most of these are actually method independent (ead_2 and c_r for sure, probably also th_2 and g_y) – AIU this does get changed back later in the same PR.

chrysn · 2026-03-05T08:06:54Z

shared/src/lib.rs

    pub ead_3: EadItems,
 }

-#[derive(Debug)]


This was a dead code struct, right? (No need to split it out, let's just confirm this so that if we wonder, we'll find our considerations.)

yes, no longer there

chrysn · 2026-03-05T08:07:53Z

lib/src/edhoc.rs

+        ProcessingM3::StatStat(inner) => r_verify_message_3_statstat(inner, crypto, valid_cred_i), // ProcessingM3::Psk(inner) => r_verify_message_3_psk(inner, crypto, valid_cred_i),
+    }
+}
+pub fn r_verify_message_3_statstat(


This shouldn't need to be pub. (Or if so, document why.).

they are pub because now they are inside edhoc/statstat.rs, otherwise they are not accessible. Changed to pub(crate)

chrysn · 2026-03-05T08:09:08Z

lib/src/edhoc.rs

    }
 }

+pub fn r_prepare_message_2_statstat(


chrysn · 2026-03-05T08:09:41Z

lib/src/edhoc.rs

+        _ => Err(EDHOCError::UnsupportedMethod),
+    }
+}
+pub fn r_parse_message_3_statstat(


chrysn · 2026-03-05T08:11:49Z

lib/src/edhoc.rs

+        _ => Err(EDHOCError::UnsupportedMethod),
+    }
+}
+// returns c_r


Is this comment even still accurate? (And if so, it'll still belong to i_parse_message_2, and should be a more specific doc comment)

old comment, will remove

chrysn · 2026-03-05T08:18:33Z

lib/src/edhoc/statstat.rs

@@ -94,36 +97,44 @@ pub fn r_parse_message_3_statstat(
 }

 pub fn r_verify_message_3_statstat(


Now this is not statstat specific any more, as it does the switching internally just again.

(And r_verify_message_3 just forwards into here)

True, I can directly pass the arguments in the r_verify_message_3 and do the match in the outer function, rather than doing it twice!

chrysn · 2026-03-05T08:32:37Z

lib/src/edhoc.rs

        }
-        // EDHOCMethod::PSK => r_prepare_message_2_psk()
+        // (EDHOCMethod::PSK, PrepareMessage2Details::Psk) =>
        _ => Err(EDHOCError::UnsupportedMethod),


That is not necessarily the error here: The error is "the details you provided do not match what we agreed on with the peer". This should at least be a comment here until we find a good plan.

chrysn · 2026-03-05T08:36:36Z

lib/src/lib.rs

-    r: BytesP256ElemLen,   // private authentication key of R
-    cred_r: Credential,    // R's full credential
+    state: ResponderStart,       // opaque state
+    r: Option<BytesP256ElemLen>, // private authentication key of R when required by method


Why is this deviating from the method-enum path?

Before message_1, the Responder does not yet know the selected EDHOC method, so it must carry identity material. For StatStat, that includes the private r key.
You’re right that using Option plus an identity enum is redundant (I did have the RepsonderIdentity enum). I’ll switch to carrying ResponderIdentity directly and derive method-specific details at prepare_message_2 (once state.method is known), which removes the extra Option adn keeps the enum style.

chrysn · 2026-03-05T08:39:03Z

shared/src/lib.rs


+#[derive(Debug)]
+pub enum ParsedMessage2Details {
+    StatStat { id_cred_r: IdCred, ead_2: EadItems },


EAD items will be in any message 2 details. (No need for a struct I think: they can be returned side by side in the multi-value return that returns PasedMessage2Details).

1. Remove old comments (returns c_r) 2. Made r_verify_message_3 method distinction in the outer level (so r_verify_message_3_statstat does not do repeated matches) 3. Add a FIXMEE on the error type. 4. Modified r: Option<> to be method-enum, matching the current sytle and avoiding repetition (ResponderIdentity is the enum) 5. ParsedMessage2Detials: removed ead_2 6. Changed pub to pub(crate) in statstat.rs

WilliamTakeshi

Heyyy, I really like how the code is shaping up. Just wrote some ocmments, the ones that are nitpick, feel free to skip them :))

lib/src/edhoc.rs

lib/src/lib.rs

lakers-c/src/initiator.rs

WilliamTakeshi · 2026-03-05T15:30:17Z

lakers-c/src/lib.rs

+    pub kind: ProcessingM2MethodSpecificsKindC,
    pub mac_2: BytesMac2,
+    pub id_cred_r: IdCred,
+}


I think here its supposed to be

#[derive(Debug)] #[repr(C)] pub enum ProcessingM2MethodSpecificsKindC { Pm2StatStat { pub mac_2: BytesMac2, pub id_cred_r: IdCred, }, }

because mac2 and id_cred_r will only exists for the StatStat, right?

If not, then mac_2 and id_cred_r shouldn't be inside the method_specifics field.

maybe its different because is the C version? I dont know

Yes, I am not sure how to do it in C. The problem is that in C there is no tagged enum with payload, as in Rust. Check commit 3e405f8 where i am trying to replicate it using the union + kind.

lakers-c/src/lib.rs

lakers-python/src/responder.rs

lib/src/edhoc/statstat.rs

lib/src/edhoc.rs

lib/src/lib.rs

shared/src/lib.rs

1. Follow the same pattern for InitiatorIdentity (similar to RepsonderIdentity) instead of making i: Option<> 2. Fix in lakers-python/src/repsonder.rs: minimize the use of ? by extracting the value only once 3. Avoid wildcard imports 4. Add FIXME for the error type. Need to find a good one 5. Removing unnecesary method in ProcessedM2

Using struct+union to replicate the enum style in Rust

WilliamTakeshi · 2026-03-10T12:45:55Z

lakers-c/src/lib.rs

+            method_specifics: ProcessedM2MethodSpecificsC {
+                kind: ProcessedM2MethodSpecificsKindC::Prm2StatStat,
+                data: ProcessedM2MethodSpecificsDataC {
+                    statstat: core::mem::ManuallyDrop::new(ProcessedM2StatStatC {}),


I only see ManuallyDrop::new, but no ManuallyDrop::drop. As far as I know, when using ManuallyDrop, the inner value must eventually be dropped manually (e.g., with ManuallyDrop::drop or ptr::drop_in_place); otherwise, it will leak.

In this specific case, ProcessedM2StatStatC {} is an empty type with no destructor, so there is nothing to drop and no leak from that field. The reason why ManuallyDrop is used is that it's the safe way to put fields in a union, especially for future non-trivial payloads, as in PSK.

I am still not sure about using ManuallyDrop. In this case its fine because is a zero-sized field, but on the PSK will have to manually drop it, right?

I think the right path is to have something that derives Copy. @chrysn can you check this and these new changes? It would be great to have this (and the other one) merged soon.

I think that if we want to have Copy then we need CredentialC to also implement Copy.
We could also avoid using the union, but I am not sure how to do the enum-style in C otherwise

WilliamTakeshi

I didn't take a look on the C code much, because I know there is some discussion if we pause the C bindings for now.

lakers-c/src/lib.rs

lib/src/edhoc.rs

lib/src/edhoc/statstat.rs

lib/src/lib.rs

- Adding a SAFETY comment in lakers-c/src/lib - Adding a FIXME to define a new error type (method_specifics doesnt match with the what was agreed) - Correcting prk_4e3m and prk_3e2m to avoid matching on symmetric credential in statstat (since now it is only the stat) - Remove _ead_2, since it is used

chrysn

We're converging in many areas, but I still think that the bulk move into edhoc/statstat.rs is excessive, especially seeing that it just leads to duplication later in #415.

Minimizing that duplication is not just something we should aim for because we strive for code quality, but because given that we're the first and AFAIK so far only implementation that has a method >3, will also help LAKE people become better at understanding what typical things that a method customizes are. We won't set those in stone, but our code is also a visual guideline for method developers for what methods such as 4 have changed so far.

Before you go on an editing spree, can we discuss (also possibly in chat or a video call, not sure yet when I'll be around) what good paths forward without too much disruption are? One option I'd consider is to acknowledge that a split-out refactoring step has served its purpose well, we've hashed out a lot in here, but in the interest of not making your processing needlessly complex (undoing some of the move-into-statstat would be hell of a rebase) we can continue on #415 (once we're sure not to lose anything from in here), refactor things back into a less duplicated state there, and then continue working on that view of the change as a whole.

chrysn · 2026-03-15T01:28:25Z

lib/src/edhoc.rs

+            r_prepare_message_2_statstat(state, crypto, cred_r, r, c_r, cred_transfer, ead_2)
+        }
+        // (EDHOCMethod::PSK, PrepareMessage2Details::Psk) =>
+        // FIXME: it is not an error, but more a lack of agreement between peers.


Continuing from https://github.com/lake-rs/lakers/pull/406/changes#r2888495250 so I can change the text here:

Suggested change

// FIXME: it is not an error, but more a lack of agreement between peers.

// FIXME: it is not a runtime error, but a programming error: The library user has seen

// StatStat details in message 1 and still provided other details creating message 2.

chrysn · 2026-03-15T01:33:28Z

lib/src/lib.rs

+#[derive(Debug)]
+pub enum InitiatorIdentity {
+    StatStat { i: BytesP256ElemLen },
+    Psk,


Suggested change

Psk,

// Psk,

chrysn · 2026-03-15T01:33:34Z

lib/src/lib.rs

+#[derive(Debug)]
+pub enum ResponderIdentity {
+    StatStat { r: BytesP256ElemLen },
+    Psk,


Suggested change

Psk,

// Psk,

chrysn · 2026-03-15T01:49:06Z

lib/src/edhoc/statstat.rs

+    ProcessingM1, ProcessingM2, ProcessingM2MethodSpecifics, ProcessingM3,
+    ProcessingM3MethodSpecifics, WaitM2, WaitM3, WaitM3MethodSpecifics, WaitM4,
+};
+use lakers_shared::Crypto as CryptoTrait;


The functions in here IMO still do way more than they should, as evidenced by a diff between lib/src/edhoc/psk.rs and lib/src/edhoc/statstat.rs the in #415:

Those easily share 20 lines in a row in many of the functions, typically at the start and end. (Sometimes that's just masked by different choices of early return and thus indentation, eg. starting from compute prk_out in r_verify_message_3_*).

Refactoring to support future methods

89482dd

ElsaLopez133 force-pushed the refactor-multi-method-support branch 2 times, most recently from 7204516 to af4dbe6 Compare February 23, 2026 11:51

Added helper functions where method-states are used because of hax er…

97097f9

…rors

ElsaLopez133 force-pushed the refactor-multi-method-support branch from af4dbe6 to 97097f9 Compare February 23, 2026 11:55

Added helper functions for all functions for clarity

179f90d

For each function we match on state and create method-specific fiunctions. For example, we have r_prepare_message_2 and we match based on emthod to either use t_prepare_message_2_statstat or r_prepare_message_2_psk

ElsaLopez133 marked this pull request as ready for review February 23, 2026 15:53

WilliamTakeshi approved these changes Feb 23, 2026

View reviewed changes

lib/src/edhoc.rs Outdated Show resolved Hide resolved

lib/src/edhoc.rs Outdated Show resolved Hide resolved

ElsaLopez133 force-pushed the refactor-multi-method-support branch 2 times, most recently from 9976f2b to f54f4df Compare February 23, 2026 16:52

Adding a separate submodule for _statstat functions

69792a9

ElsaLopez133 force-pushed the refactor-multi-method-support branch from f54f4df to 69792a9 Compare February 23, 2026 17:01

chrysn reviewed Feb 23, 2026

View reviewed changes

ElsaLopez133 force-pushed the refactor-multi-method-support branch 2 times, most recently from a18f9ce to a5de4f7 Compare February 24, 2026 08:55

Replacing outer/inner states for method_specific entry

35820e5

We avoid repetition of shared-values among methods This affects ProcessingM2 and ProcessingM3

ElsaLopez133 force-pushed the refactor-multi-method-support branch from a5de4f7 to 35820e5 Compare February 24, 2026 09:10

WilliamTakeshi reviewed Feb 25, 2026

View reviewed changes

shared/src/lib.rs Outdated Show resolved Hide resolved

ElsaLopez133 added 2 commits February 26, 2026 14:20

Removing method from state ProcessingM3 and ProcessingM2

4bdbbc2

Removing method from state ProcessingM3 and ProcessingM2

d767859

Adding enum in prepare_message_2 to define inputs based on method

ElsaLopez133 force-pushed the refactor-multi-method-support branch 8 times, most recently from bc90672 to dd223be Compare February 27, 2026 14:11

ElsaLopez133 force-pushed the refactor-multi-method-support branch from dd223be to ee44c6e Compare February 27, 2026 14:15

Correcting verify_message_2 for future PSK

46e2ecc

ElsaLopez133 force-pushed the refactor-multi-method-support branch from 7dc5c68 to 46e2ecc Compare March 4, 2026 07:38

ElsaLopez133 mentioned this pull request Mar 4, 2026

Adding psk method #410

Closed

chrysn reviewed Mar 5, 2026

View reviewed changes

WilliamTakeshi self-requested a review March 5, 2026 09:20

chrysn mentioned this pull request Mar 5, 2026

python: release as 0.6.1-rc1 #412

Merged

ElsaLopez133 force-pushed the refactor-multi-method-support branch from d218a10 to 0af8c8b Compare March 5, 2026 13:22

ElsaLopez133 force-pushed the refactor-multi-method-support branch from 0af8c8b to c63cd51 Compare March 5, 2026 13:51

WilliamTakeshi reviewed Mar 5, 2026

View reviewed changes

ElsaLopez133 force-pushed the refactor-multi-method-support branch from 57596ab to c93fffd Compare March 5, 2026 20:11

Modifying lakers-c

3e405f8

Using struct+union to replicate the enum style in Rust

WilliamTakeshi reviewed Mar 10, 2026

View reviewed changes

WilliamTakeshi self-requested a review March 11, 2026 08:45

chrysn requested changes Mar 15, 2026

View reviewed changes

		@@ -94,36 +97,44 @@ pub fn r_parse_message_3_statstat(
		}

		pub fn r_verify_message_3_statstat(

	// FIXME: it is not an error, but more a lack of agreement between peers.
	// FIXME: it is not a runtime error, but a programming error: The library user has seen
	// StatStat details in message 1 and still provided other details creating message 2.

Conversation

ElsaLopez133 commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamTakeshi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElsaLopez133 Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElsaLopez133 Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamTakeshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElsaLopez133 Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamTakeshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ElsaLopez133 commented Feb 23, 2026 •

edited

Loading

WilliamTakeshi left a comment •

edited

Loading

ElsaLopez133 Mar 5, 2026 •

edited

Loading

ElsaLopez133 Mar 5, 2026 •

edited

Loading

ElsaLopez133 Mar 12, 2026 •

edited

Loading