Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle non-consecutive tag numbers in Sigma3 certificate extensions #97

Merged
merged 6 commits into from
Sep 25, 2023

Conversation

ssnover
Copy link
Contributor

@ssnover ssnover commented Sep 17, 2023

This addresses #96 by adding param unordered for FromTLV for Extensions such that the incoming order does not matter.

@ivmarkov
Copy link
Contributor

@ssnover Hmmmm, this looks like a temp fix to me, that would only address proper decoding of the Extensions struct, so that CASE session establishment works with SmartThings.

However, isn't the real, proper fix to touch the proc-macros, so that FromTLV works even if the tags are not ordered sequentially?

@ssnover
Copy link
Contributor Author

ssnover commented Sep 18, 2023

I think the issue is primarily one of data representation. Extensions here is a struct, but in TLV it is a list of variants, each of which should appear at most once. I don't think the uniqueness constraint could reliably be represented in the macro as that requirement is not necessarily there for all lists.

However, I can see why this representation of a struct of Options would be chosen for a microcontroller, it's much easier to work with on the device side this way. Needing to scan a list for a value like the subject key ID each time would be annoying, but so would needing to define one struct for deserializing and another to copy all the data into for access purposes, especially when this is happening on every CASE session.

@ivmarkov
Copy link
Contributor

ivmarkov commented Sep 18, 2023

I think the issue is primarily one of data representation. Extensions here is a struct, but in TLV it is a list of variants, each of which should appear at most once. I don't think the uniqueness constraint could reliably be represented in the macro as that requirement is not necessarily there for all lists.

However, I can see why this representation of a struct of Options would be chosen for a microcontroller, it's much easier to work with on the device side this way. Needing to scan a list for a value like the subject key ID each time would be annoying, but so would needing to define one struct for deserializing and another to copy all the data into for access purposes, especially when this is happening on every CASE session.

I'm not sure I follow. For me, the problem of modeling that some/all items in the list are optional is completely orthogonal to the problem (that we actually have), that these items might pop up in arbitrary order. Which needs fixing on the proc macro level that currently probably assumes that these appear in increasing tag ID order in the tlv.

@ivmarkov
Copy link
Contributor

ivmarkov commented Sep 18, 2023

@ssnover Actually, there is already a support for unordered tag IDs. So the fix should be as simple as adding an unordered annotation to the struct.

@ssnover
Copy link
Contributor Author

ssnover commented Sep 20, 2023

That did the trick all right, thanks for the pointer!

@kedars kedars merged commit 7ef08ad into project-chip:main Sep 25, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants