Core functions for the lake-authz draft #110

geonnave · 2023-10-04T14:16:49Z

This PR will implement the core functions for EAD message preparation and processing with focus on U and V:

prepare EAD_1 (U)
process EAD_1 (V)
prepare Voucher Request (V)
prepare Voucher Response (W)
prepare EAD_2 (U)
process EAD_2 (V)
handle VREQ and VRES in stateless and stateful operation modes
integration with the main crate
pass CI, a.k.a. works with all features and all crypto backends

Draft link: https://www.ietf.org/archive/id/draft-selander-lake-authz-03.html

Architecture:

U                           V                                       W
|                           |                                       |
|      EDHOC message_1      |                                       |
+-------------------------->|                                       |
|  (EAD_1 = LOC_W, ENC_ID)  |                                       |
|                           |                                       |
|                           |        Voucher Request (VREQ)         |
|                           +-------------------------------------->|
|                           |       (message_1, ?opaque_state)      |
|                           |                                       |
|                           |        Voucher Response (VRES)        |
|                           |<--------------------------------------+
|                           |  (message_1, Voucher, ?opaque_state)  |
|                           |                                       |
|      EDHOC message_2      |                                       |
|<--------------------------+                                       |
|     (EAD_2 = Voucher)     |                                       |
|                           |                                       |
|                           |                                       |
|      EDHOC message_3      |                                       |
+-------------------------->|                                       |
|                           |                                       |

- includes test vectors - also updates test vectors to reuse keys from lake-traces-07

geonnave · 2023-10-12T17:26:57Z

@malishav it still needs polishing but all core functions of the draft work, so it could receive a first pass of review. Please focus on the edhoc-ead-zeroconf crate, since the integration with the rest of edhoc-rs is currently broken due to some API changes. (that is why the CI totally fails; on the other hand, cargo test for the edhoc-ead-zeroconf crate passes).

geonnave · 2023-10-12T17:30:08Z

Also, for reference, I used this notebook to create the traces / test vectors.

geonnave · 2023-10-13T11:10:33Z

It looks better now, but I think it will not be fully working with all features and possible configurations by the end of the day.

Since I will leave for PTO until 24 Oct, I would like to merge this today (if the review goes well), even if I have to adapt the CI with feature/exclude flags.

geonnave · 2023-10-13T13:11:12Z

Wohoo, CI passing!

malishav

Thanks for providing this PR @geonnave ! I left some inline comments, the major red flag that I see is parsing code which does not check the actual number of bytes received and may panic. Other than that, I think there is a misconception on the length of the voucher and some excessive allocations. See inline for more details.

malishav · 2023-10-23T15:48:40Z

ead/edhoc-ead-zeroconf/src/lib.rs

+}
+
+// NOTE: can we import this from the edhoc-rs main crate?
+fn edhoc_kdf(


yes, we should definitely have a way of calling the KDF function frm this code without code duplication

Perhaps, we can do the same as with encode_enc_structure and move this to a helper module?

For the time being I moved just the logic for the info struct to a helpers module in consts.

Part of the reason is to avoid having consts depend on the cryptographic backends.
Another part is that this is now only a single small duplicated function, so in my view it still does not justify changing the dependencies in consts.
Nevertheless I added a TO-DO (🙃), that we should address as more cases like this arise.

malishav · 2023-10-23T16:06:48Z

ead/edhoc-ead-zeroconf/src/lib.rs

+    (),
+> {
+    let mut message_1 = EdhocMessageBuffer::new();
+    let mut voucher = EdhocMessageBuffer::new();


This is an excessive allocation for a voucher. Voucher is always of size MAC_LENGTH which corresponds to the MAC length used in EDHOC. Since we only support a single cipher suite for now, this should use MAC_LENGTH.

I changed this for consistency, but note that since we need to put the voucher in an EADItem struct, we sill need to allocate one large buffer since the EADItem.value is an EdhocMessageBuffer.

malishav · 2023-10-23T16:18:08Z

ead/edhoc-ead-zeroconf/src/lib.rs

+        return Err(());
+    }
+
+    message_1.len = voucher_response.content[2] as usize;


It seems that you are not checking anywhere how many bytes are actually in the received voucher_response, i.e. the value of voucher_response.len. This means that your code may panic when it receives a malformed Voucher Response or worse take values from the internal buffer at random. This code should be rewritten to check remaining voucher_response.len before actually accessing voucher_response.content array.

Added size verifications to all parse_* routines.

malishav · 2023-10-23T16:19:34Z

ead/edhoc-ead-zeroconf/src/lib.rs

+    ead_1_value: &Option<EdhocMessageBuffer>,
+) -> Result<(EdhocMessageBuffer, EdhocMessageBuffer), ()> {
+    let value = ead_1_value.unwrap();
+    let loc_w: EdhocMessageBuffer = value.content[4..4 + value.content[3] as usize]


Same comment as above: before accessing value.content, we need to check how long it is...

malishav · 2023-10-23T16:21:26Z

ead/edhoc-ead-zeroconf/src/lib.rs

+    }
+
+    message_1.len = vreq.content[2] as usize;
+    message_1.content[..message_1.len].copy_from_slice(&vreq.content[3..3 + message_1.len]);


Same comment on accessing vreq.content[] as above

ead/edhoc-ead-zeroconf/src/lib.rs

malishav · 2023-10-24T08:40:34Z

ead/edhoc-ead-zeroconf/src/lib.rs

+
+    // IV_1 = EDHOC-Expand(PRK, info = (1, h'', AES_CCM_KEY_LEN), length)
+    let mut iv_1: BytesCcmIvLen = [0x00; AES_CCM_IV_LEN];
+    // NOTE (FIXME?): here we actually generate AES_CCM_KEY_LEN bytes, but then we only use AES_CCM_IV_LEN of them (next line)


This is strange. Looking at draft-ietf-lake-authz:

The derivation of IV_1 = EDHOC-Expand(PRK, info, length) uses the following input to the info struct (see Section 4.2):

info_label = 1
context = h'' (the empty CBOR string)
length is length of nonce of the EDHOC AEAD algorithm in bytes

According to this, length should be set to AES_CCM_IV_LEN.

ead/edhoc-ead-zeroconf/src/lib.rs

malishav · 2023-10-24T08:52:27Z

ead/edhoc-ead-zeroconf/src/lib.rs

+}
+
+const EAD_ENC_STRUCTURE_LEN: usize = 2 + 8 + 3;
+fn encode_enc_structure(ss: u8) -> [u8; EAD_ENC_STRUCTURE_LEN] {


Is this the same code as in lib/src/edhoc.rs, function encode_enc_structure ? If so, why not move it like a helper function together with CBOR-related helpers? Jsut rename the module to something more appropriate

It is not the same, the input in this case is just ss: u8 while in lib/src/edhoc.rs the input is th_3: &BytesHashLen.

malishav · 2023-10-24T08:55:50Z

ead/edhoc-ead-zeroconf/src/lib.rs

+pub fn r_process_ead_1(ead_1: &EADItem, message_1: &BufferMessage1) -> Result<(), ()> {
+    let opaque_state: Option<EdhocMessageBuffer> = None; // TODO: receive as parameter
+
+    if ead_1.label != EAD_ZEROCONF_LABEL {


Will this be checked somehow beforehand, i.e. in the library code invoking the EAD handler? If yes, this could be an replaced with an assertion

It is not checked before, since the library is agnostic of the EAD handlers. Thus I think we can keep the if check.

geonnave · 2023-10-27T08:17:04Z

From my side, this is ready to merge.

malishav

Partial review below, as discussed offline.

ead/edhoc-ead-zeroconf/src/lib.rs

malishav · 2023-11-02T13:47:25Z

ead/edhoc-ead-zeroconf/src/lib.rs

+}
+
+fn build_enc_id(
+    prk: &BytesHashLen, // ephemeral key of U


The comment here confused me. prk is not an ephemeral key, but the ECDH secret derived from the ephemeral key of U and static key of W. The code seems correct, the comment does not.

As discussed, was a reminiscent comment from before a refactoring. Removed.

malishav · 2023-11-02T14:01:31Z

ead/edhoc-ead-zeroconf/src/lib.rs

+    let mut message_1 = EdhocMessageBuffer::new();
+    let mut voucher: BytesEncodedVoucher = Default::default();
+
+    let array_byte = voucher_response.content[0];


You are accessing voucher_response.content[0] here before checking whether the received length, voucher_response.len, is greater than zero. Are you ensuring that the function can be called only when voucher_response.len is >0? If yes, then you could have an assert assert!(voucher_response.len > 0). If not, the whole block of code lines 365-409 should be conditioned on if(voucher_reponse.len > 0) IMO.

malishav · 2023-11-02T14:07:13Z

ead/edhoc-ead-zeroconf/src/lib.rs

+        return Err(());
+    }
+
+    let message_1_len = voucher_response.content[2] as usize;


Here, you accessed voucher_response.content[2] before having checked if there are enough bytes in the buffer according to voucher_response.len. That may panic if voucher_response.len is only 1 byte.

As discussed, we will address this issue in a separate PR.

geonnave force-pushed the core-ead-zeroconf branch 6 times, most recently from 3fe2ddf to 656dced Compare October 5, 2023 15:38

geonnave added the new feature label Oct 9, 2023

geonnave added 3 commits October 9, 2023 14:49

feat[zeroconf]: encode ead_1

65f28a7

wip: build ENC_ID

c90481e

feat: build ENC_ID

c3fcfa6

geonnave force-pushed the core-ead-zeroconf branch 4 times, most recently from e677de2 to ca45d49 Compare October 10, 2023 11:33

feat: add test vector playground

e1bb540

geonnave force-pushed the core-ead-zeroconf branch from ca45d49 to e1bb540 Compare October 10, 2023 11:36

geonnave added 3 commits October 10, 2023 17:02

wip: fixing ead_1 preparation

0eaf146

fix: build_enc_id generates correct result

54d4e5e

fix: i_prepare_ead_1 pass tests

54813d8

geonnave force-pushed the core-ead-zeroconf branch from 3196c8e to 54813d8 Compare October 11, 2023 08:54

feat: build voucher_request and test with TV

0b25c8d

geonnave force-pushed the core-ead-zeroconf branch 2 times, most recently from 56c83d1 to e756401 Compare October 12, 2023 10:32

feat: parse voucher request, encode voucher input

3a182c5

- includes test vectors - also updates test vectors to reuse keys from lake-traces-07

geonnave force-pushed the core-ead-zeroconf branch from e756401 to 3a182c5 Compare October 12, 2023 11:49

geonnave added 4 commits October 12, 2023 15:21

feat: compute mac and encode voucher

10ed108

feat: build voucher request

f365e06

feat: prepare ead_2

f680b8c

feat: process ead 2 at U

9b6daa6

geonnave force-pushed the core-ead-zeroconf branch from 005d114 to 9b6daa6 Compare October 12, 2023 17:20

geonnave marked this pull request as ready for review October 12, 2023 17:20

geonnave added 3 commits October 13, 2023 11:01

feat: make opaque state optional

abac7db

feat: add tests for stateful (default) operation of V

7d33db3

refactor: move cbor helpers to consts crate

9930ec6

geonnave requested a review from malishav October 13, 2023 11:11

feat: make main crate tests pass

65f73d0

geonnave force-pushed the core-ead-zeroconf branch from 97e6817 to 65f73d0 Compare October 13, 2023 13:03

malishav reviewed Oct 24, 2023

View reviewed changes

refactor: make sure .unwraps are safe

c846dfe

geonnave force-pushed the core-ead-zeroconf branch from c764496 to 442c774 Compare October 25, 2023 11:53

refactor: validate sizes on parsing

439c326

geonnave force-pushed the core-ead-zeroconf branch from 442c774 to 439c326 Compare October 25, 2023 11:53

geonnave added 5 commits October 25, 2023 14:02

refactor: use constants instead of magic numbers

774c8ca

fix: voucher has MAC_LENGTH bytes

e994bf2

refactor: use smaller arrays for voucher

125df8d

fix: adjust iv length according to draft

dce19eb

refactor: move encoding of info to helpers module

bb20772

geonnave force-pushed the core-ead-zeroconf branch from d53d18d to bb20772 Compare October 26, 2023 09:21

malishav reviewed Nov 2, 2023

View reviewed changes

chore: remove outdated comment

d07c5ee

malishav merged commit 8fc2763 into openwsn-berkeley:main Nov 3, 2023
24 checks passed

geonnave deleted the core-ead-zeroconf branch January 17, 2024 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core functions for the lake-authz draft #110

Core functions for the lake-authz draft #110

geonnave commented Oct 4, 2023 •

edited

Loading

geonnave commented Oct 12, 2023

geonnave commented Oct 12, 2023

geonnave commented Oct 13, 2023 •

edited

Loading

geonnave commented Oct 13, 2023

malishav left a comment

malishav Oct 23, 2023

malishav Oct 24, 2023

geonnave Oct 26, 2023

malishav Oct 23, 2023

geonnave Oct 25, 2023

malishav Oct 23, 2023

geonnave Oct 25, 2023 •

edited

Loading

malishav Oct 23, 2023

malishav Oct 23, 2023

malishav Oct 24, 2023

geonnave Oct 26, 2023

malishav Oct 24, 2023

geonnave Oct 26, 2023

malishav Oct 24, 2023

geonnave Oct 26, 2023

geonnave commented Oct 27, 2023

malishav left a comment

malishav Nov 2, 2023

geonnave Nov 2, 2023

malishav Nov 2, 2023

malishav Nov 2, 2023

malishav Nov 3, 2023

Core functions for the lake-authz draft #110

Core functions for the lake-authz draft #110

Conversation

geonnave commented Oct 4, 2023 • edited Loading

geonnave commented Oct 12, 2023

geonnave commented Oct 12, 2023

geonnave commented Oct 13, 2023 • edited Loading

geonnave commented Oct 13, 2023

malishav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geonnave Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geonnave commented Oct 27, 2023

malishav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geonnave commented Oct 4, 2023 •

edited

Loading

geonnave commented Oct 13, 2023 •

edited

Loading

geonnave Oct 25, 2023 •

edited

Loading