Credentials also supporting symmetric keys #294
Conversation
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 openwsn-berkeley#274
There was a problem hiding this comment.
@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).
geonnave
force-pushed
the
new-creds
branch
2 times, most recently
from
ff9c761
to
4f2348f
Compare
borrowing code from @chrysn's PR openwsn-berkeley#274
geonnave
force-pushed
the
new-creds
branch
4 times, most recently
from
3a5b5de
to
85b4875
Compare
|
Not all tests pass yet, but this is now ready for review:
In terms of code, something that still bothers me is the overuse of Also as stated in the first comment, this borrows ideas and code from #274. |
| initiator | ||
| .set_identity( | ||
| I.try_into().expect("Wrong length of initiator private key"), | ||
| cred_i.clone(), |
There was a problem hiding this comment.
Why do you need to clone cred_i here?
There was a problem hiding this comment.
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)
| // 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)?; |
There was a problem hiding this comment.
I am a little confused with a difference between id_cred.as_full_value() and id_cred.as_encoded_value()?
There was a problem hiding this comment.
Pushed a commit with better documentation on both functions, hope it suffices?
| } | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Could you comment on why we don't need this block any more?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good, thanks for the explanation!
shared/src/cred.rs
Outdated
| /// 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 { |
There was a problem hiding this comment.
Do we want this in the current PR?
| } else { | ||
| Err(EDHOCError::ParsingError) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
For now I documented it as experimental and moved the tests to a dedicated test_experimental module.
geonnave
merged commit cd97b1b
into
openwsn-berkeley:crypto-method-agility
includes breaking changes from PRs openwsn-berkeley#284 and openwsn-berkeley#294
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.