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

Matching a Reference Value to Accepted Claims Set #107

Merged
merged 30 commits into from
Sep 8, 2023

Conversation

andrew-draper
Copy link
Collaborator

This PR provides a description of how I see verifier behaviour in comparing a Reference Value against the Accepted Claims Set
It skirts around the undecided question of how to group Reference Values, Endorsements etc together as an input to this matching.
This PR partly resolves issue 71

Copy link
Collaborator

@nedmsmith nedmsmith left a comment

Choose a reason for hiding this comment

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

multiple comments provided inline

draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
Endorsements in the group are added to the Accepted Claims Set
(see {{sec-add-to-acs}}).

If any Reference Value in a group does not match then this does not affect
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If any Reference Value in a group does not match then this does not affect
If any Reference Value in a group does not match the Evidence that is appropriate for the group then this does not affect

draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
nedmsmith and others added 2 commits August 14, 2023 16:28
Co-authored-by: Yogesh Deshpande <yogesh.deshpande@arm.com>
Co-authored-by: Yogesh Deshpande <yogesh.deshpande@arm.com>
henkbirkholz and others added 3 commits September 6, 2023 16:17
Co-authored-by: Ned Smith <ned.smith@intel.com>
Co-authored-by: Ned Smith <ned.smith@intel.com>
Co-authored-by: Ned Smith <ned.smith@intel.com>
Copy link
Collaborator

@nedmsmith nedmsmith left a comment

Choose a reason for hiding this comment

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

LGTM

draft-ietf-rats-corim.md Outdated Show resolved Hide resolved
draft-ietf-rats-corim.md Show resolved Hide resolved
draft-ietf-rats-corim.md Show resolved Hide resolved
draft-ietf-rats-corim.md Show resolved Hide resolved

A Reference Value consists of an `environment-map` plus a `measurement-map`. In the
`reference-values-triple-record` these are packaged together. In other triples multiple
Reference Values are represented more compactly by letting one `environment-map`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Reference Values are represented more compactly by letting one `environment-map`
where multiple measurements may be represented by one environment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree this is not beautiful, but I am not convinced yours is better

Copy link
Collaborator

Choose a reason for hiding this comment

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

None of the statements are correct. As of CDDL in the current spec, a Reference Value triple has one environment and corresponding one measurement-map So considering this fact, I would re-word the statement as

Copy link
Collaborator

@nedmsmith nedmsmith Sep 7, 2023

Choose a reason for hiding this comment

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

It is reasonable to consider multiple triples having the same environment-map but using different triples to arrive at a set of measurements that belong to the environment. An internal representation would be described by an environment with multiple measurements.

Overall, the reader is confused about what the internal representation is and how the Verifier processes it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not use the word triples then. I was confused by the statement.

I would re-phrase it then to:

Internally the Verifier can combine all the measurements belonging to the same environment together or something similar

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not use the word triples then. I was confused by the statement.

I would re-phrase it then to:

Internally the Verifier can combine all the measurements belonging to the same environment together or something similar

I agree that we should have conventions for describing the internal representation without referring to specific CDDL / encodings. The challenge is doing this without being nebulous. Possibly, a strategy is to describe the internal representation by describing the CDDL that instantiated it. e.g., "...the environment (instantiated by environment-map)..."

Copy link
Member

Choose a reason for hiding this comment

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

does this thread of comments require an issue to be raised before merging?

Copy link
Collaborator

@nedmsmith nedmsmith Sep 7, 2023

Choose a reason for hiding this comment

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

does this thread of comments require an issue to be raised before merging?

No. see issue #144

draft-ietf-rats-corim.md Show resolved Hide resolved
draft-ietf-rats-corim.md Show resolved Hide resolved
draft-ietf-rats-corim.md Show resolved Hide resolved
@yogeshbdeshpande
Copy link
Collaborator

@andrew-draper : Important Point:

You need to rebase your Pull Request to the tip of the file on the main branch.

Otherwise key fixes in these sections (like Feedback from RATS) and other work items from @nedmsmith will be wiped out!

@yogeshbdeshpande
Copy link
Collaborator

@andrew-draper : Important Point:

You need to rebase your Pull Request to the tip of the file on the main branch.

Otherwise key fixes in these sections (like Feedback from RATS) and other work items from @nedmsmith will be wiped out!

And please let me know, if you need me to rebase if you face any issues in rebasing.

Signed-off-by: Yogesh Deshpande <yogesh.deshpande@arm.com>
Copy link
Collaborator

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

As all open issues are tracked via github, hence approving the PR!

@ietf-rats-wg ietf-rats-wg deleted a comment from nedmsmith Sep 7, 2023
Value does not match.

The Verifier iterates over the digests array in the reference value, locating
algorithms which are present in the Reference Value and the Accepted Claims
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK with me (obviously)

@andrew-draper
Copy link
Collaborator Author

Add first text describing how to match a Reference Value against Accepted Claims Set. Fixes issue #71

@andrew-draper andrew-draper merged commit 48bdd67 into main Sep 8, 2023
2 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.

5 participants