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.

Sign up for GitHub

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
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6375c6
feat(rust/signed_doc): check protected metadata field support
no30bit May 14, 2025
68309dc
fix docs
no30bit May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions rust/signed_doc/src/metadata/extra_fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,25 @@ impl ExtraFields {
self.category_id
}

/// Counts non empty fields. These fields reflect what the
/// serialized/deserialized object would look like, as the empty ones are skipped.
#[must_use]
pub(super) fn count_serialized_fields(&self) -> u8 {
[
self.doc_ref().is_some(),
self.template().is_some(),
self.reply().is_some(),
self.section().is_some(),
!self.collabs().is_empty(),
self.brand_id().is_some(),
self.election_id().is_some(),
self.category_id().is_some(),
]
.into_iter()
.map(u8::from)
.sum()
}

/// Fill the COSE header `ExtraFields` data into the header builder.
pub(super) fn fill_cose_header_fields(
&self, mut builder: coset::HeaderBuilder,
Expand Down
41 changes: 41 additions & 0 deletions rust/signed_doc/src/metadata/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,26 @@ impl Metadata {
protected: &coset::ProtectedHeader, report: &ProblemReport,
) -> Self {
let metadata = InnerMetadata::from_protected_header(protected, report);
let unsupported = count_unsupported_cose_keys(&protected.header, &metadata);
if unsupported != 0 {
report.functional_validation(
&format!("Unsupported protected fields are not allowed ({unsupported} found)"),
"When deserializing protected header",
);
}
Self::from_metadata_fields(metadata, report)
}
}

/// Counts how many COSE keys header contains that weren't deserialized into
/// [`InnerMetadata`].
fn count_unsupported_cose_keys(
original_header: &coset::Header, deserialized_metadata: &InnerMetadata,
) -> usize {
count_relevant_cose_keys(original_header)
.saturating_sub(usize::from(deserialized_metadata.count_serialized_fields()))
}

impl InnerMetadata {
/// Converting COSE Protected Header to Metadata fields, collecting decoding report
/// issues.
Expand Down Expand Up @@ -211,6 +227,31 @@ impl InnerMetadata {

metadata
}

/// Counts non empty fields. These fields reflect what the
/// serialized/deserialized object would look like, as the empty ones are skipped.
///
/// Includes [`ExtraFields::count_serialized_fields`].
#[must_use]
fn count_serialized_fields(&self) -> u8 {
[
self.doc_type.is_some(),
self.id.is_some(),
self.ver.is_some(),
self.content_type.is_some(),
self.content_encoding.is_some(),
]
.into_iter()
.map(u8::from)
.sum::<u8>()
.saturating_add(self.extra.count_serialized_fields())
}
}

/// 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 •
edited
Loading

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 •
edited
Loading

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.

usize::from(header.content_type.is_some()).saturating_add(header.rest.len())
}

impl Display for Metadata {
Expand Down
Loading