openwsn-berkeley / lakers

EDHOC implemented in Rust, optimized for microcontrollers, with bindings for C and Python.
https://crates.io/crates/lakers
BSD 3-Clause "New" or "Revised" License
12 stars 10 forks source link

r_parse_message_3: Tolerate unparsable credential-by-value #271

Closed chrysn closed 1 month ago

chrysn commented 1 month ago

This enables using CWTs as values, provided that in the same step as where the user expands a credential-by-kid into a full credential, they also parse any non-CCS (or differently shaped CCS) values such as a CWT into the credential (left as it is), the key and the key ID.

This builds on #270 and addresses its usability caveat described in https://github.com/openwsn-berkeley/lakers/issues/269#issuecomment-2113512458

I still think we may want to elaborate the CredentialRpk type into differentiating at the type level whether it is valid (has KID, key and credential) or just partial (in the shapes "we parsed it already, it is valid", "we only have the KID" and "we only have the credential value but no keys", possibly others in the future), but that's API rework, and this enables things with the API as we have it.

geonnave commented 1 month ago

But what if the credential is indeed invalid?

I think this changes the semantics of the API, as now the application cannot know if the credential was parsed or not.

One hacky way would be to add a .parsed? method to CredentialRPK, and which checks for (1) having a value but (2) not having kid or public key, which would indicate an unparsed credential.

Right now I can't think of a better solution without an API re-design.

chrysn commented 1 month ago

I don't think that Lakers can be expected to differentiate an invalid credential from one that it just does not understand.

The way it currently exposes properties rather than accessors makes the differentiation between "has no key" and "the key is empty" hard, and improving that may be API change -- we can make the properties private, maybe turn some of them into an Option, and do all kinds of enhancements.

To enhance this change in isolation, we could add an is_complete() method (deprecating reference_only() to use ! is_complete() instead) that also checks whether a key is set. But that is for the convenience of the users to know whether or not they need to provide an actual (possibly hand-crafted) credential. If the user does not check, I'd assume that the same happens as if there was a KID only and the user doesn't upgrade it: The credential, key and KID don't match what is in the EDHOC context, and the operation fails.

geonnave commented 1 month ago

Ok. The spec indeed says in Section 5.3.3: When and how to perform authentication is up to the application.

Since it is not obvious why we are ignoring the error in the proposed change, there could be a comment in the else clause about leaving the credential processing to the application.

chrysn commented 1 month ago

Good point, comment was added.