-
Notifications
You must be signed in to change notification settings - Fork 33
Verify measure, host data, and report data function #117
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
Verify measure, host data, and report data function #117
Conversation
Reviewer's GuideThis PR extends the verification CLI by adding three new subcommands (measurement, host_data, report_data). Each subcommand reads an attestation report, decodes a hex-encoded input string, checks its length, converts it to a fixed-size byte array, and compares it against the corresponding field in the report. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The three
verify_*functions share nearly identical logic; consider extracting a reusable helper to parse, decode, and compare hex data to reduce duplication. - Enum variants
Host_dataandReport_datadon’t follow Rust’s CamelCase naming convention—rename them toHostDataandReportData(and update Clap command names accordingly). - Several argument doc comments (e.g. “Run the TCB Verification Exclusively.”) look copy-pasted and don’t describe the new host_data/report_data commands; please update them to match each command’s purpose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The three `verify_*` functions share nearly identical logic; consider extracting a reusable helper to parse, decode, and compare hex data to reduce duplication.
- Enum variants `Host_data` and `Report_data` don’t follow Rust’s CamelCase naming convention—rename them to `HostData` and `ReportData` (and update Clap command names accordingly).
- Several argument doc comments (e.g. “Run the TCB Verification Exclusively.”) look copy-pasted and don’t describe the new host_data/report_data commands; please update them to match each command’s purpose.
## Individual Comments
### Comment 1
<location> `src/verify.rs:26` </location>
<code_context>
/// Verify the attestation report.
Attestation(attestation::Args),
+
+ ///Verify the measurment
+ Measurement(measure::Args),
+
</code_context>
<issue_to_address>
Typo in 'measurment' should be 'measurement'.
Fixing the typo ensures consistent terminology in the codebase.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
///Verify the measurment
Measurement(measure::Args),
=======
/// Verify the measurement
Measurement(measure::Args),
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/verify.rs:783` </location>
<code_context>
+
+ let expected_measurement = att_report.measurement;
+
+ //Check to make sure the length of actual_measurement is 48
+ if args.measurement.len() != 96 {
+ return Err(anyhow::anyhow!("Expected 96 characters, got {}, invalid measurement length.", args.measurement.len()));
</code_context>
<issue_to_address>
Comment refers to 'actual_measurement' before it is defined.
Clarify that the comment refers to the hex string length, not the byte array, to prevent misunderstanding.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
//Check to make sure the length of actual_measurement is 48
=======
// Check to make sure the length of the measurement hex string is 96 characters (representing 48 bytes)
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/verify.rs:809` </location>
<code_context>
+ return Err(anyhow::anyhow!("Measurements did not match: \n expected {} \n got {}", hex::encode(expected_measurement), hex::encode(actual_measurement)));
+ }
+
+ //match statement
+ if !quiet {
+ println!("Measurement matches.");
</code_context>
<issue_to_address>
Comment 'match statement' is misleading as there is no match statement here.
Please revise or delete the comment to accurately reflect the code.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
//match statement
if !quiet {
println!("Measurement matches.");
=======
// print success message if not quiet
if !quiet {
println!("Measurement matches.");
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/verify.rs:851` </location>
<code_context>
+
+ let expected_host_data = att_report.host_data;
+
+ //Check to make sure the length of actual_host_data is 32
+ if args.host_data.len() != 64 {
+ return Err(anyhow::anyhow!("Expected 64 characters, got {}, invalid host_data length.", args.host_data.len()));
</code_context>
<issue_to_address>
Comment refers to 'actual_host_data' before it is defined.
Specify that the length check is performed on the hex string representation, not the decoded byte array.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
//Check to make sure the length of actual_host_data is 32
if args.host_data.len() != 64 {
return Err(anyhow::anyhow!("Expected 64 characters, got {}, invalid host_data length.", args.host_data.len()));
}
=======
// Check that the hex string representation of host_data is 64 characters (32 bytes when decoded)
if args.host_data.len() != 64 {
return Err(anyhow::anyhow!("Expected 64 characters, got {}, invalid host_data length.", args.host_data.len()));
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Can you resolve the cargo clipply and the cargo fmt issues first please. |
61249cd to
5f660b0
Compare
@DGonzalezVillal I went ahead and cleaned up all the clippy and fmt errors. |
|
Thanks for this PR—those checks are genuinely useful for remote attestation; verifying these fields is part of the RA baseline. Two quick interface notes (happy to help either way): Input format (hex vs file path)Input format (hex vs file) snpguest report report.bin report-data.bin -rwhich emits a random binary This preserves round-trips with current output and avoids hex-only UX. In my opinion, it is better to start by accepting a single format/source. That lets this land quickly. We can follow up with multi-format/multi-source support. (Related issue #100) Command shapeConceptually, "signature/metadata verify" + "fields match expectations" is one flow. I would lean toward folding these into the existing command: snpguest verify attestation \
report.bin \
--report-data report-data.bin \
--host-data ... \
--measurement ...(Thin wrapper subcommands could still delegate to the same impl if you would like.) I am happy to draft these changes if useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some code suggestions
You bring up good points, and I agree there should be a clean way of passing this information in for validation. We do have precedent in other parts of our tooling where we pass measurements as a string. From the format the user provides, we can determine whether the string is hex or Base64. It’s not the perfect approach, but it avoids the need to handle a separate file for each parameter a user wants to verify, which could be tedious. For now, I’d be fine supporting the string in either hex or Base64 format. Looking ahead, we could also consider an extended option, like a JSON file, where all the fields to be validated are provided together. That said, if you believe the separate files approach is better, Arvind can update the code to reflect that instead.
After thinking about it more closely, I think a cleaner way forward might be to extend the existing attestation verification functionality rather than generating three separate subcommands that essentially take the same parameters. |
a7f9ee5 to
0f2abde
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code mostly good. Just a couple of quick things.
The one last thing is that @hyperfinitism was saying maybe this functionality makes more sense on the "verify attestation" subcommand, instead of making this functionality it's own subcommand.
@tylerfanelli do you have any thoughts
0f2abde to
9939dc4
Compare
|
@tylerfanelli thoughts on this |
|
Logically these commands fit more with the verify attestation subcommand. I think that should be extended to accommodate this. |
5d2da6d to
c1374b2
Compare
@tylerfanelli Sorry for the delay. Just made that change. Could you take another look? |
src/verify.rs
Outdated
| fn decode_hex_or_base64(input: &str) -> Result<Vec<u8>> { | ||
| // Look for "0x" at beginning. If it exists, treat as a hex. | ||
| if let Some(hex_str) = input.strip_prefix("0x") { | ||
| return Ok(Vec::from_hex(hex_str)?); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all hex has the 0x prefix. We should just standardize on one format (for user input, hex is preferred IMO) and deserialize the input as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other commands (generating id-block) people could pass a measurement in either BASE64 or hex since the measurement could be outputted in either of those formats. So I had been telling Arvind that we could ask user's to distinguish between those two formats using 0x. I know that's not standard, but we have done it in the past. This pr #119 continues following that logic.
Maybe we want a different way of telling the different types of inputs? OR just force people to always use hex? It's a little hard because we have so many different structs to handle with different ways of checking them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hex is standard for user input. base64 is more compact, so works well when humans aren't inputting. I'd prefer to remove the prefix and require hex for all arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove Base64 as inputs then and only support base64 outputs for relevant fields.
Maybe we want to wait for this PR then: #119
Which standardizes inputs in the tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at #119 I noticed that the function created in that PR is not applicable here, because it's only created for u32 and u64, and can't handle the fields that we're creating this functionality for. So I've reused my original helper function decode_hex_or_decimal().
b2434ff to
e1106f9
Compare
10602d9 to
5660718
Compare
docs/snpguest.1.adoc
Outdated
| -m, --measurement Optional string to verify the measurement field in the attestation report | ||
| -d, --host-data Optional string to verify the host-data field in the attestation report | ||
| -r, --report-data Optional string to verify the report-data in the attestation report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| -m, --measurement Optional string to verify the measurement field in the attestation report | |
| -d, --host-data Optional string to verify the host-data field in the attestation report | |
| -r, --report-data Optional string to verify the report-data in the attestation report | |
| -m, --measurement provide and expected measurement to verify the measurement field in the attestation report | |
| -d, --host-data provide host-data to verify the host-data field in the attestation report | |
| -r, --report-data provide report-data to verify the report-data in the attestation report |
Making it a flag implies the parameter is already optional
| The user can use the [-m, --measurement] flag to verify that the measurement in the attestation | ||
| report matches the expected measurement value. | ||
| The user can use the [-d, --host-data] flag to verify that the host-data in the attestation | ||
| report matches the expected host-data value. | ||
| The user can use the [-r, --report-data] flag to verify that the report-data in the attestation | ||
| report matches the expected report-data value. | ||
| If the optional flags are not passed, just the signature will be verified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The user can use the [-m, --measurement] flag to verify that the measurement in the attestation | |
| report matches the expected measurement value. | |
| The user can use the [-d, --host-data] flag to verify that the host-data in the attestation | |
| report matches the expected host-data value. | |
| The user can use the [-r, --report-data] flag to verify that the report-data in the attestation | |
| report matches the expected report-data value. | |
| If the optional flags are not passed, just the signature will be verified. | |
| The user can use the [-m, --measurement] flag to verify that the measurement in the attestation report matches the expected measurement value (prefix with 0x for hex, without prefix it assumes decimal values). | |
| The user can use the [-d, --host-data] flag to verify that the host-data in the attestation report matches the expected host-data value (prefix with 0x for hex, without prefix it assumes decimal values). | |
| The user can use the [-r, --report-data] flag to verify that the report-data in the attestation report matches the expected report-data value (prefix with 0x for hex, without prefix it assumes decimal values). |
wording
README.md
Outdated
| # Verify Attestation Measurement only | ||
| snpguest verify attestation ./certs attestation-report.bin --measurement | ||
| # Verify Attestation host-data only | ||
| snpguest verify attestation ./certs attestation-report.bin --host-data | ||
| # Verify Attestation report-data only | ||
| snpguest verify attestation ./certs attestation-report.bin --report-data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this examples you would need to pass an example of what the data would look like. You don't just pass the flag, you also need to pass the string that would be used to verify.
…the attestation report. Adding the ability to verify attestation to verify measurement, host data, and report data attributes from the attestation report. Signed-off-by: Arvind Kumar <[email protected]>
Adding instructions and changes to documentation on how to use the optional features measure, host-data, and report-data features added to the verify attestation command. Signed-off-by: Arvind Kumar <[email protected]>
5660718 to
99b0326
Compare
|
@tylerfanelli ptal |
Creating functionality to verify the measurement host data and report data attributes from the attestation report.
Summary by Sourcery
Introduce three new verification commands to compare provided hex-encoded measurement, host data, and report data values against the corresponding fields in an attestation report.
New Features:
verify measurementsubcommand to validate a supplied hex measurement against the attestation reportverify host_datasubcommand to validate a supplied hex host_data value against the attestation reportverify report_datasubcommand to validate a supplied hex report_data value against the attestation report