Skip to content

feat(rust/signed-doc): check protected metadata field support #322

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

no30bit
Copy link
Contributor

@no30bit no30bit commented May 14, 2025

Description

Implements reporting of unsupported fields found in a protected header. Fields are deemed unsupported if existing logic discarded them by not deserialising into corresponding Metadata fields (if any).

I find mismatch between the number of protected fields and the number of fields that end up in Metadata. To this end there's new counting functions.

I prefer this approach over validating each key individually against a list of supported keys, because the later doesn't by itself solve duplication potential and the later is always more intense computationally.

Related Issue(s)

Closes #316

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@no30bit no30bit self-assigned this May 14, 2025
@no30bit no30bit added bug Something isn't working review me PR is ready for review do not merge yet PR is not ready to merge yet F14-RC2 labels May 14, 2025
@no30bit no30bit added this to Catalyst May 14, 2025
@no30bit
Copy link
Contributor Author

no30bit commented May 14, 2025

I'll add the unit tests if the implementation is fine with everyone

@no30bit no30bit changed the title feat(rust/signed_doc): check protected metadata field support feat(rust/signed-doc): check protected metadata field support May 14, 2025
@no30bit no30bit moved this from New to 👀 In review in Catalyst May 14, 2025
@no30bit no30bit marked this pull request as ready for review May 14, 2025 14:09
Copy link
Contributor

Test Report | ${\color{lightgreen}Pass: 319/319}$ | ${\color{red}Fail: 0/319}$ |


/// Counts non empty fields, excluding those that are **not** used when deserializing
/// [`Metadata`].
fn count_relevant_cose_keys(header: &coset::Header) -> usize {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not agree with the naming and the description for this function.
if I am not mistaken, but this function returns the number of all header fields no ?
If so you should also take into account other header fields like alg, kid etc.

Copy link
Contributor Author

@no30bit no30bit May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function returns the number of all header fields

Not exactly. As I mentioned in the doc comment, the function excludes those that are not used when deserialising.

The naming idea is that alg and kid aren't related to metadata and I focus only on metadata here. I draw conclusion that they aren't, because they don't end up in the Metadata type, thus irrelevant at this level.

This is intentional, because my implementation depends on the structure of the Metadata type. If I counted alg and kid, I would get N+2 while InnerMetadata::from_protected_header doesn't use those fields and our InnerMetadata doesn't contain them (by N I mean the number of InnerMetadata fields in a protected header). Then when I count the number of fields that are successfully deserialised (M, where M ≤ N), I would always have a failing case M ≤ N < N + 2, because the invariant is N = M.

Copy link
Contributor Author

@no30bit no30bit May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, the validity of kid and alg are guaranteed by coset implementation of <coset::Header as AsCborValue>::from_cbor_value. There these reserved fields are explicitly matched and converted.

Any fields that aren't reserved are put into the Header::rest field. Our metadata fields except for the content_type all lie in there. So this is the only place to check for dups. But I account for the content_type field by adding it to the relevant count, as it is a part of InnerMetadata but not a part of the Header::rest.

@github-project-automation github-project-automation bot moved this from 👀 In review to 🔖 Ready in Catalyst May 15, 2025
@no30bit no30bit marked this pull request as draft May 19, 2025 10:19
@no30bit
Copy link
Contributor Author

no30bit commented May 19, 2025

Abandoned and superseded by #334 and #332 solutions

@no30bit no30bit removed the review me PR is ready for review label May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working do not merge yet PR is not ready to merge yet F14-RC2
Projects
Status: 🔖 Ready
Development

Successfully merging this pull request may close these issues.

Signed Document non recognisable header fields capturing
2 participants