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

Certification declaration parsing is future-hostile #11082

Closed
bzbarsky-apple opened this issue Oct 27, 2021 · 5 comments
Closed

Certification declaration parsing is future-hostile #11082

bzbarsky-apple opened this issue Oct 27, 2021 · 5 comments
Assignees
Labels
attestation spec Mismatch between spec and implementation V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

The code added in #10825 requires that there be no tags after kTag_DACOriginVendorId. That means future spec versions will not be able to add anything there.

Why are we doing that?

Proposed Solution

Stop requiring CHIP_END_OF_TLV there. Allow CHIP_NO_ERROR as well, and probably CHIP_ERROR_UNEXPECTED_TLV_ELEMENT.

@emargolis
Copy link
Contributor

I don't think it is a good idea to allow TLV elements in the CD, which cannot be interpreted by the code. This is why the code is written this way, which doesn't allow unrecognized elements. Any future formats CD formats will require new SKD version, which knows to interpret new format.

The spec doesn't clearly define format_version element. It looks that we can claim that this parser only supports CD format_version == 1. For any future format_version the parser should be updated.

@tcarmelveilleux ^^

@bzbarsky-apple
Copy link
Contributor Author

So "new" certification declarations are guaranteed to be be backwards-incompatible with downrev commissioners?

If that's what we are doing, why are we bothering with TLV at all? We could have a more compact fixed-field encoding here....

@bzbarsky-apple
Copy link
Contributor Author

In either case, if the expectation is that no other tags are allowed, the spec needs to explicitly call that out. That is NOT the general expectation for TLV things.

@woody-apple
Copy link
Contributor

This looks resolved?

@bzbarsky-apple
Copy link
Contributor Author

Looks like #11838 fixed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attestation spec Mismatch between spec and implementation V1.0
Projects
None yet
Development

No branches or pull requests

4 participants