Refactoring adding psk method#415
Conversation
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
We avoid repetition of shared-values among methods This affects ProcessingM2 and ProcessingM3
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
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
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
- 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
Adding PSK specific functions Adding coap examples Adding edhoc/psk.rs module
1fd46c3 to
aaaa14c
Compare
Fixing unused variables Fixing lakers-c and lakers-python Adding FIXME for error declaration Fixing coap examples Matching in edhoc.rs instead of edhoc/psk.rs Fix lakers-c: Pm2Psk defined twice
(cherry pick from pr-psa-memory-error)
6499b4d to
d0192ec
Compare
- simplify compute_th_3 to use Option<&[u8]> for CRED_R - unify encode_plaintext_2 with Option<(&[u8], &BytesMac2)> - unify encode_plaintext_3 with Option<(&[u8], &BytesMac3)> - unify message_3 AAD handling with Option<(&[u8], &[u8], &[u8])>
refactor edhoc method-specific message processing Keep match-based dispatch in edhoc.rs and move shared flow out of statstat/psk for: - r_prepare_message_2 - r_parse_message_3 - r_verify_message_3 - i_parse_message_2 - i_verify_message_2 - i_prepare_message_3
| ParsedMessage2Details::StatStat { id_cred_r } => id_cred_r, | ||
| ParsedMessage2Details::Psk {} => IdCred::new(), | ||
| }; | ||
| *id_cred_r_out = id_cred_r; |
There was a problem hiding this comment.
on the PSK case, why write an empty id_cred_r in id_cred_r_out. I thought we would just do nothing.
There was a problem hiding this comment.
So I addressed that by adding a flag has_id_cred_r_out. The flag now tells whether ID_CRED_R is present. I still initialize id_cred_r_out to a default empty value in the PSK case just to avoid leaving output memory undefined on the C side, but maybe this is not strictly needed?
There was a problem hiding this comment.
Good question, I don't know what is the right thing to do on C, but I guess better than adding a flag has_id_cred_r_out it should be something like method and be a 3 for Statstat and 4 for Psk.
And yes, reading again I think your code is correct, they sohuld still have the empty value initialized in the PSK case, we just ignore it.
WilliamTakeshi
left a comment
There was a problem hiding this comment.
Besides the comments I put here,
- can you also check if nRF52 code still runs??? I don't have one with me right now :((
- write an full e2e test like test_handshake(). It will show if everything works and give an example for future. Also you can use to test, like the one that I said when message3 is empty you can force the responder to crash
|
|
||
| pub fn set_identity(&mut self, i: BytesP256ElemLen, cred_i: Credential) { | ||
| self.i = Some(i); | ||
| self.i = Some(InitiatorIdentity::StatStat { i }); |
There was a problem hiding this comment.
do you need a set_identity for PSK? or its the default?
There was a problem hiding this comment.
Yes, we still need set_identity for PSK. In the PSK case there is no P-256 i value to provide, but set_identity is still how the initiator records that it is using InitiatorIdentity::Psk and stores cred_i, which is required later when preparing message 3.
3875b4d to
e4943ee
Compare
16b9d13 to
65f53f8
Compare
Address review feedback around PSK support - Modify lakers-c/initiator flow to hanlde both stat/stat and PSK. - Add initiator-side tests in lakers-c for stat/stat and PSK parse/verify behavior. - Add explicit has_id_cred_r in lakers-c for optional ID_CRED_R. - Update set_identity to take InitiatorIdentity, so PSK can set identity state without a stat/stat i value. - Add a full end-to-end PSK handshake test - Replace silent error drops with propagated EDHOCErrors. - Fix parse_message_3 / PSK parsing to stop assuming one-byte ID_CRED_PSK. - Add proper CBOR byte-string header encoding for lengths beyond 255.
65f53f8 to
0755dc5
Compare
- Add a dedicated PSK native C example and modified the Makefile. - Fix the lakers-c FFI EAD item conversion to respect the populated item count - Add a symmetric credential constructor to the C FFI so the PSK example -
Make the Python EDHOC bindings method-aware for both initiator and responder. The initiator binding can now be constructed for stat-stat or PSK, chooses the correct identity shape in verify_message_2, and parses credentials according to the selected EDHOC method. The responder binding now mirrors the Rust API more closely by making the static responder key optional: passing no `r` selects PSK, while providing `r` selects stat-stat. It also parses credentials according to the negotiated method during verification. Update the Python tests to the new responder constructor API
eb89d72 to
cb918be
Compare
WilliamTakeshi
left a comment
There was a problem hiding this comment.
There is a binary committed on examples/lakers-c-native/lakers_c_native_psk. Probably committed by mistake.
Its getting really good :))
| #[new] | ||
| fn new(r: Vec<u8>, py: Python<'_>, cred_r: super::AutoCredential) -> PyResult<Self> { | ||
| #[pyo3(signature = (cred_r, r=None))] | ||
| fn new(py: Python<'_>, cred_r: super::AutoCredential, r: Option<Vec<u8>>) -> PyResult<Self> { |
There was a problem hiding this comment.
Here we are breaking the API because we are changing the order, we have to document this somewhere, because it will break people updating the Python API. I don't know if this is fine.
There was a problem hiding this comment.
True, I did not think of this. Maybe we could make the arguments keyword only? This way postitional argument order is no longer relevant?
| #[new] | ||
| fn new(r: Vec<u8>, py: Python<'_>, cred_r: super::AutoCredential) -> PyResult<Self> { | ||
| #[pyo3(signature = (cred_r, r=None))] | ||
| fn new(py: Python<'_>, cred_r: super::AutoCredential, r: Option<Vec<u8>>) -> PyResult<Self> { |
There was a problem hiding this comment.
Also another issue is, Option<Vec<u8>> "represents" 3 states:
- Not provided -
None - Provided wo data -
Some([]) - Provided with data -
Some([1,2])
Its not clear the difference between the user using the API with 1. or 2.
But I am fine keeping like this so we can merge PSK and thinking the APIs later :))
- Fixed encode_ciphertext_3a to allow lengths > 23 - Fixed parse_message_3 to decode CBOR bstr correctly - Added comments on psk.rs - lakers-python: removed Python FFI panics - lakers-c: removed duplicated method field from EdhocInitiator
|
Where are we at with this PR? I still see TODOs in the PR description. @ElsaLopez133 can you give us an update? |
|
I believe the PR is ready to review. Here is a concise summary of what was done:
What would need to be done is to add the Responder logic in the C bindings, but this would be a separate PR I believe (as it is neither there for the stat-stat protocol flow in the main branch) |
Can you update the PR description with this text? thanks |
WilliamTakeshi
left a comment
There was a problem hiding this comment.
LGTM
@chrysn , when you have time, can you take a look? I think we can merge this one and slowly improve some parts
Adding PSK method after refactoring.
edhoc.rsis kept as the main EDHOC protocol file and depending on the method we branch to different functions that are stored in edhoc/statstat.rsandedhoc/psk.rs. Therefore,edhoc/statstat.rsandedhoc/psk.rs` contain only the parts that differ between the Static-Static and PSK methods.These method-specific files return result structs back to
edhoc.rs, so the central flow can continue without duplicating the shared protocol logic.We store method-specific fields in
enumssuch asWaitM3MethodSpecifics,ProcessingM2MethodSpecifics,ProcessedM2MethodSpecifics, andProcessingM3MethodSpecifics. These enums allow the common state structs to carry only the extra data required by the selected method. The use ofenumswas chosen to avoid havingOptionvalues.