Integrating EDHOC-PSK as EDHOC_METHOD=4#399
Integrating EDHOC-PSK as EDHOC_METHOD=4#399ElsaLopez133 wants to merge 19 commits intolake-rs:mainfrom
Conversation
|
Thanks for providing the PR! Could you go ahead and resolve the conflicts with the main? And feel free to ping us when you feel it's ready for review. |
bfd7646 to
cc487e1
Compare
WilliamTakeshi
left a comment
There was a problem hiding this comment.
Thanks for the great work! I don't understand fully the PSK, so maybe some comments here are more me asking questions than providing review.
Also, can you remove old comments? I saw there there is lots of old experiments, older versions of functions as comments, it make a bit harder to read.
| fn hkdf_extract_psk(&mut self, salt: &BytesHashLen, ikm: &BytesP256ElemLenPSK) -> BytesHashLen { | ||
| // TODO | ||
| // TODO generalize if salt is not provided | ||
| let output = self.hmac_sha256(&mut ikm.clone()[..], *salt); |
There was a problem hiding this comment.
Just a small nit: you can write &mut ikm.clone() instead of &mut ikm.clone()[..], since the slice coercion happens automatically.
(just for you to know, don't worry about changing, its fine)
| fn hkdf_extract_psk(&mut self, salt: &BytesHashLen, ikm: &BytesP256ElemLenPSK) -> BytesHashLen { | ||
| // TODO | ||
| // TODO generalize if salt is not provided | ||
| let output = self.hmac_sha256(&mut ikm.clone()[..], salt); |
There was a problem hiding this comment.
Here we are creating extra allocation.
You can just self.hmac_sha256(ikm, salt);. you probably copied from the other example, but the requeriments for this hmac_sha256 are different, so no need to clone or deref
| } | ||
|
|
||
| impl<'a, Crypto: CryptoTrait> EdhocInitiatorWaitM4<Crypto> { | ||
| pub fn process_message_4( |
There was a problem hiding this comment.
here we changing from process_message_4 to parse_message_4 but still a couple places where we still use process_message_4. on examples-nrf52840 for example
shared/src/lib.rs
Outdated
| pub const SUITES_LEN: usize = 9; | ||
| pub const SUPPORTED_SUITES_LEN: usize = 1; | ||
| pub const EDHOC_METHOD: u8 = 3u8; // stat-stat is the only supported method | ||
| pub const EDHOC_METHOD: u8 = 3u8; // stat-stat: 3u8, psk:4u8 |
There was a problem hiding this comment.
remove the unused consts
| }; | ||
| // compute ciphertext_2 | ||
| let plaintext_2 = encode_plaintext_2(c_r, id_cred_r.as_encoded_value(), &mac_2, &ead_2)?; | ||
| let plaintext_2 = match state.method { |
There was a problem hiding this comment.
I dont know if its possible, but maybe try transform ProcessingM1::method to be the EDHOCMethod, then it clears lots of future .into() since we only check once in the start of the process. It will give us lots of compiletime guarantees
There was a problem hiding this comment.
And the EDHOCError::UnsupportedMethod will happen way earlier in the process
shared/src/lib.rs
Outdated
| #[derive(Debug)] | ||
| #[derive(Debug, Clone)] | ||
| pub struct ProcessingM1 { | ||
| pub method: u8, |
There was a problem hiding this comment.
could these u8 parameters be replaced with EDHOCMethod? Using the enum would provide stronger type guarantees and prevent invalid values at compile time.
Same for WaitM2, WaitM3, ProcessingM2, ....
| method: u8, | ||
| plaintext_3: &BufferPlaintext3, | ||
| ) -> Result<(IdCred, BytesMac3, EadItems), EDHOCError> { | ||
| ) -> Result<(Option<IdCred>, Option<BytesMac3>, EadItems), EDHOCError> { |
There was a problem hiding this comment.
I’m not sure returning (Option<_>, Option<_>, EadItems) is the best direction.
This makes impossible states representable (e.g. Some(_), None). If method were an enum instead of a u8, the API could encode these invariants more safely and avoid those invalid combinations.
shared/src/lib.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug)] | ||
| #[derive(Clone, Debug)] |
There was a problem hiding this comment.
Since this represents a protocol state, deriving Clone may not be ideal. We want to model as a object that is meant to move forward, and not duplicated.
Also, this clone is unused. Maybe you added when you were debugging but not its safe to remove?
(same comment for all other states)
| defmt-or-log = { version = "0.2.1", default-features = false } | ||
| log = { version = "0.4", optional = true } | ||
| defmt = { version = "0.3", optional = true } | ||
| hex = "0.4" |
There was a problem hiding this comment.
This crate is only used for println! some messages.
instead of
println!("message_2 : 0x{}", encode(message_2.as_slice()));
we can just
println!("message_2 : 0x{:02x?}", message_2);
and its almost the same, without the use of external crates
lib/src/lib.rs
Outdated
| pub const X: [u8; 32] = hex!("09972DFEF1EAAB926EC96E8005FED29F70FFBF4E361C3A061A7ACDB5170C10E5"); | ||
| pub const G_X: [u8; 32] = hex!("7EC68102940602AAB548539BF42A35992D957249EB7F1888406D178A04C912DB"); | ||
| pub const Y: [u8; 32] = hex!("1E1C8F2DF1AA7110B39F33BA5EA8DCCF31411EB33D4F9A094CF65192D335A7A3"); | ||
| pub const G_Y: [u8; 32] = hex!("ED156A6243E0AFEC9EFBAABCE8429D5AD5E4E1C432F76A6EDE8F79247BB97D83"); |
There was a problem hiding this comment.
unused consts, also hex_literal::hex is unused.
I think its safe we remove both and then we can remove the crate from lib/Cargo.toml :)
61d4749 to
ef3e502
Compare
Remove EDHOC_METHOD ocnstant and replace with a TryFrom Impl of EDHOCMEthod enum
e6f1030 to
70ba372
Compare
848a05a to
a2727d1
Compare
malishav
left a comment
There was a problem hiding this comment.
I went through the PR. It is not ready for merging. In terms of form, there are many leftovers, comments, unusual naming. Please fix those.
In terms of substance, we should discuss whether the integration of another protocol (i.e. PSK) makes sense to be done within the existing functions. If we continue adding other EDHOC methods like this, the code base will simply be unusable. I would propose a discussion with @chrysn @WilliamTakeshi and myself on another way forward. Similarly how we make the distinction on initiator and responder-specific functions, e.g. r_parse_message_3, etc, we should introduce dedicated functions for the PSK method, e.g. method_4_decode_plaintext_2, reusing the auxiliary functions to the extent possible. I am happy to hear other opinions on the best way forward.
| output | ||
| } | ||
|
|
||
| fn hkdf_extract_psk(&mut self, salt: &BytesHashLen, ikm: &BytesP256ElemLenPSK) -> BytesHashLen { |
There was a problem hiding this comment.
we don't need a new function for this. we need to generalize hkdf_extract and use that. @WilliamTakeshi any proposal how to generalize hkdf_extract to support both uses? This eventually needs to converge with the embedded-call API.
| output | ||
| } | ||
|
|
||
| fn hkdf_extract_psk(&mut self, salt: &BytesHashLen, ikm: &BytesP256ElemLenPSK) -> BytesHashLen { |
There was a problem hiding this comment.
this should not exist, see comment above
| use lakers_shared::{ | ||
| BytesCcmIvLen, BytesCcmKeyLen, BytesHashLen, BytesP256ElemLen, Crypto as CryptoTrait, | ||
| EDHOCError, EDHOCSuite, EdhocBuffer, AES_CCM_TAG_LEN, MAX_SUITES_LEN, | ||
| BytesCcmIvLen, BytesCcmKeyLen, BytesHashLen, BytesP256ElemLen, BytesP256ElemLenPSK, |
There was a problem hiding this comment.
Rename BytesP256ElemLenPSK to something that actually makes sense for the PSK.
| self.cred_i = Some(cred_i); | ||
| pub fn set_identity(&mut self, i: Option<BytesP256ElemLen>, cred_i: Credential) { | ||
| self.i = i; | ||
| self.cred_i = Some(cred_i.clone()); |
| pub fn set_identity(&mut self, i: BytesP256ElemLen, cred_i: Credential) { | ||
| self.i = Some(i); | ||
| self.cred_i = Some(cred_i); | ||
| pub fn set_identity(&mut self, i: Option<BytesP256ElemLen>, cred_i: Credential) { |
There was a problem hiding this comment.
Let's reverse the arguments in set_identity such that cred_i appears first, and i appears last.
| } | ||
| let _ = ciphertext_3a | ||
| .fill_with_slice(&message_slice[header_len..header_len + ciphertext_3a_len]); | ||
| // println!("ciphertext_3a: {:?}", ciphertext_3a); |
| let mut mac_2: BytesMac2 = [0x00; MAC_LENGTH_2]; | ||
|
|
||
| let mut decoder = CBORDecoder::new(plaintext_2.as_slice()); | ||
| trace!("decoder:{:?}", decoder); |
| mac_2[..].copy_from_slice(decoder.bytes_sized(MAC_LENGTH_2)?); | ||
| let (id_cred_r, mac_2) = match method { | ||
| EDHOCMethod::StatStat => { | ||
| let input = decoder.any_as_encoded()?; |
There was a problem hiding this comment.
can you keep the StatStat-related diff the same, i.e. so we know there are no changes to the implementation
| } | ||
| } | ||
|
|
||
| pub fn parse_message_3( |
There was a problem hiding this comment.
Could you comment on the need to introduce this function?
| Ok(id_cred) | ||
| } | ||
|
|
||
| fn decode_cbor_length(first_byte: u8) -> Result<(usize, usize), EDHOCError> { |
There was a problem hiding this comment.
don't we have a cbor decoder for this?
chrysn
left a comment
There was a problem hiding this comment.
I've only looked at the API changes so far, and there didn't get all the way through.
Apart from individual comments inline, please split this up a bit; this PR contains some huge commits that touch many places in the code. Those should be split up into workable portions. Sure, adding the method is something that can't really be split up, but ground work such as adding a method field to states can be reviewed and tested better in isolation than at the same time as when it is utilized.
As reworking a PR of that size is a tedious task, and incoming comments might mean that changes are needed anyway, as a first step I suggest you take components where a) you don't expect that they are controversial, and b) they make sense even with out PSK (such as setting the method enum top Copy, Clone and non_exhaustive) and c) the commit history rebases cleanly onto those changes into a dedicated PR.
| pub type BufferKid = EdhocBuffer<16>; // variable size, up to 16 bytes | ||
| pub type BufferIdCred = EdhocBuffer<192>; // variable size, can contain either the contents of a BufferCred or a BufferKid | ||
| pub type BytesKeyAES128 = [u8; 16]; | ||
| pub type BytesKeyKid = [u8; 4]; |
There was a problem hiding this comment.
Key ID lengths will in general vary, especially for external PSKs. So maybe use an EdhocBuffer<4>?
There was a problem hiding this comment.
It seems this file is not referenced anywhere.
| fn hkdf_extract(&mut self, salt: &BytesHashLen, ikm: &BytesP256ElemLen) -> BytesHashLen; | ||
| fn aes_ccm_encrypt_tag_8<const N: usize>( | ||
| fn hkdf_extract_psk(&mut self, salt: &BytesHashLen, ikm: &BytesP256ElemLenPSK) -> BytesHashLen; | ||
| fn aes_ccm_decrypt_tag_8<const N: usize>( |
There was a problem hiding this comment.
The changes in sequence make this needlessly hard to review; why is that changed?
I tried to find out through history, but "Fixing th_4 and th_3 in PSK mode" is a way larger single commit than anything should be.
| pub const SUPPORTED_SUITES_LEN: usize = 1; | ||
| pub const EDHOC_METHOD: u8 = 3u8; // stat-stat is the only supported method | ||
| pub const P256_ELEM_LEN: usize = 32; | ||
| pub const P256_ELEM_LEN_PSK: usize = 16; |
There was a problem hiding this comment.
What does that constant mean? (For P256_ELEM_LEN I can somehow figure it out, it's the number of bytes in a P256 scalar, but that logic doesn't take me anywhere here).
| #[derive(PartialEq, Debug)] | ||
| #[derive(PartialEq, Debug, Copy, Clone)] | ||
| #[repr(C)] | ||
| pub enum EDHOCMethod { |
There was a problem hiding this comment.
This would be the perfect time to make this #[non_exhaustive].
|
|
||
| #[derive(Debug)] | ||
| #[repr(C)] | ||
| pub struct ProcessingM4 { |
There was a problem hiding this comment.
Adding ProcessingM4 and ProcessedM4 implies to me that there is any but the trivial processing necessary to go from WaitM4 to completed, but the current draft section 3.2 only allows for the credential to be chosen in message 3, so at the time the message 4 comes back, the EDHOC stack should already have all the information needed and not need to call out again to the application.
(I could be missing something, but then that should be in the commit message of the isolated change that adds those states, whereas it's now in a huge single commit).
This PR introduces an initial implementation of EDHOC-PSK (authentication method 4), following the specification on draft-ietf-lake-edhoc-psk-06.
The implementation is tested on the coap examples in examples/coap/
TODOs: