Credentials also supporting symmetric keys by geonnave · Pull Request #294 · lake-rs/lakers

geonnave · 2024-07-01T08:45:15Z

Work in progress. Leverages some ideas from #274.

Built on top of #284, and supposed to be merged into the feature branch crypto-method-agility.

will support more types of credentials, including different types of COSE_Key's the handling of id_cred's is inspired by chrysn's PR lake-rs#274

geonnave

@ElsaLopez133 thanks for pushing on parse_ccs_psk. Some comments below.

Please format the code with cargo fmt (or even better, install the pre-commit tool to do that automatically -- see the file .pre-commit-config.yaml).

    // Why do they add +3 and +1 in CredentialPRK::parse

This comment can be sent over e.g. slack rather than in the code

Also please use a commit message following the pattern we use in the project (see previous commits to get a sense of it).

@chrysn

borrowing code from @chrysn's PR lake-rs#274

geonnave · 2024-07-04T16:18:50Z

Not all tests pass yet, but this is now ready for review:

new IdCred type that handles ID_CRED_x by value, by kid, etc., including encoding/decoding with the compact serialization when applicable
new Credential type that supports EC2 and Symmetric keys (and thus can work with PSK, cc @ElsaLopez133), and plays along with the IdCred type
fixes throughout the library to correctly compute MACs based on the right selected encoding of ID_CRED_x (before, we were always computing the MAC over an ID_CRED_x by reference, even when it was sent by value, as reported by @chrysn in MACed ID_CRED_x is always {4: "k"} #272).
now the reference to a credential (e.g. a kid) can be a byte string, i.e. not limited to u8 anymore

In terms of code, something that still bothers me is the overuse of map_err in cred.rs, but I haven't yet found a better solution.

Also as stated in the first comment, this borrows ideas and code from #274.

malishav

Congrats for pulling this one off! What remains to be done for this to be in a state where we can merge it? I left some comments inline, mainly for my understanding.

malishav · 2024-07-05T07:48:47Z

examples/lakers-no_std/src/main.rs

+        initiator
+            .set_identity(
+                I.try_into().expect("Wrong length of initiator private key"),
+                cred_i.clone(),


Why do you need to clone cred_i here?

It's because we need access to cred_i later in credential_check_or_fetch(Some(cred_i), id_cred_i).

(but maybe this could be a reference, will take a look)

malishav · 2024-07-05T07:58:39Z

lib/src/edhoc.rs

-
    // compute ciphertext_2
-    let plaintext_2 = encode_plaintext_2(c_r, &id_cred_r, &mac_2, &ead_2)?;
+    let plaintext_2 = encode_plaintext_2(c_r, id_cred_r.as_encoded_value(), &mac_2, &ead_2)?;


I am a little confused with a difference between id_cred.as_full_value() and id_cred.as_encoded_value()?

Pushed a commit with better documentation on both functions, hope it suffices?

malishav · 2024-07-05T08:08:37Z

lib/src/edhoc.rs

-                    }
-                }
-            };
-


Could you comment on why we don't need this block any more?

According to Section 5.4.3 of RFC 9528, ID_CRED_I is just made available to the application, which will process it. So even if it is a credential by value, we don't need to parse it here.

In addition, now the logic to parse ID_CRED_I is embedded in decode_plaintext_3, which calls IdCred::from_encoded_value. That's why we don't need to use different constructs for e.g. CompactKid or FullCredential.
Note that this only parses ID_CRED_I: from a (potentially compact-encoded) reference or value, e.g. kid / {14: kccs}, to the actual value of the ID_CRED_I, e.g.{4: kid} / {14: kcss}. Then, resolving the kid to a credential or verifying the syntax of the kcss is left to the application.

Sounds good, thanks for the explanation!

malishav · 2024-07-05T08:19:49Z

shared/src/cred.rs

+    /// Creates a new CCS credential with the given bytes and a pre-shared key
+    ///
+    /// This type of credential is to be used with the under-development EDHOC method PSK.
+    pub fn new_ccs_psk(bytes: BufferCred, symmetric_key: BytesKeyAES128) -> Self {


Do we want this in the current PR?

malishav · 2024-07-05T08:29:48Z

shared/src/cred.rs

I believe the PSK parsing routine should be added on top of the current PR as a separate branch, which we can merge once the PSK method becomes more stable

For now I documented it as experimental and moved the tests to a dedicated test_experimental module.

includes breaking changes from PRs lake-rs#284 and lake-rs#294

geonnave added 4 commits June 27, 2024 16:05

creds: new more versatile credentials module

f0a263d

will support more types of credentials, including different types of COSE_Key's the handling of id_cred's is inspired by chrysn's PR lake-rs#274

creds: updates on new cred module

59b50d5

creds: refactor into simpler structs

ff45dd6

creds: add parse methods

40a06ad

geonnave marked this pull request as draft July 1, 2024 08:48

Implement parse_ccs_psk

b48d826

geonnave commented Jul 1, 2024

View reviewed changes

geonnave force-pushed the new-creds branch from b48d826 to 3473ef3 Compare July 2, 2024 15:57

creds(WIP): implement parse_ccs_psk

7f35e58

geonnave force-pushed the new-creds branch from 3473ef3 to 7f35e58 Compare July 2, 2024 15:57

creds: simplify api and add tests

f378163

geonnave force-pushed the new-creds branch from 9e29ce3 to f378163 Compare July 2, 2024 16:30

ElsaLopez133 added 2 commits July 3, 2024 14:43

creds: Correcting parse_css_psk

ea91f50

creds: Correcting parse_css_psk

e5defab

geonnave force-pushed the new-creds branch 2 times, most recently from ff9c761 to 4f2348f Compare July 3, 2024 13:26

creds: use new struct for IdCred

0ce1959

borrowing code from @chrysn's PR lake-rs#274

geonnave force-pushed the new-creds branch from 4f2348f to 0ce1959 Compare July 3, 2024 13:26

creds: new id_cred struct improvements

a6792fc

geonnave force-pushed the new-creds branch 4 times, most recently from 3a5b5de to 85b4875 Compare July 4, 2024 14:05

creds: apply new id_cred struct across codebase

6d449b3

geonnave force-pushed the new-creds branch from 85b4875 to 6d449b3 Compare July 4, 2024 14:08

creds: apply new credential struct across codebase

e4e1b62

geonnave force-pushed the new-creds branch from 86a7d35 to e4e1b62 Compare July 4, 2024 15:46

creds: cleaning up after refactor

d378208

geonnave force-pushed the new-creds branch from 692f742 to d378208 Compare July 4, 2024 16:05

creds: replace old cred module with newer

23e276c

geonnave marked this pull request as ready for review July 4, 2024 16:10

malishav reviewed Jul 5, 2024

View reviewed changes

geonnave added 2 commits July 5, 2024 11:25

creds: improve id_cred encoding docs

03f7d43

creds: annotate that psk/symmetric code is experimental

71e9f6a

geonnave merged commit cd97b1b into lake-rs:crypto-method-agility Jul 5, 2024

geonnave added the type:new feature label Oct 4, 2024

geonnave added a commit to geonnave/lakers that referenced this pull request Oct 4, 2024

release: bump version to 0.7.0

a216c24

includes breaking changes from PRs lake-rs#284 and lake-rs#294

geonnave mentioned this pull request Oct 4, 2024

Release: bump version to 0.7.0 #310

Merged

Conversation

geonnave commented Jul 1, 2024

Uh oh!

geonnave left a comment

Choose a reason for hiding this comment

Uh oh!

geonnave commented Jul 4, 2024

Uh oh!

malishav 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants