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

Add Verifier for ReportData #25

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Add Verifier for ReportData #25

merged 2 commits into from
Mar 6, 2023

Conversation

nick-mobilecoin
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #25 (c7dddd5) into nick/mr_signer (86f7c82) will increase coverage by 0.26%.
The diff coverage is 96.77%.

@@                Coverage Diff                 @@
##           nick/mr_signer      #25      +/-   ##
==================================================
+ Coverage           92.84%   93.10%   +0.26%     
==================================================
  Files                   2        2              
  Lines                 433      464      +31     
==================================================
+ Hits                  402      432      +30     
- Misses                 31       32       +1     
Impacted Files Coverage Δ
verifier/src/lib.rs 85.29% <ø> (ø)
verifier/src/report_body.rs 96.34% <96.77%> (+0.04%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

verifier/src/report_body.rs Outdated Show resolved Hide resolved
@jcape jcape removed the request for review from NotGyro March 2, 2023 08:51
@jcape jcape self-requested a review March 2, 2023 08:51
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

This is fine for now, but we should also add a ReportDataMaskVerifier for compatibility with EPID's sake.

Right now in EPID land, report_data contains two things: 32 bytes of static noise identity public key, and 32 bytes of app-specific data we want to attest to (e.g. in consensus, this is a block signing public key, in fog ingest this, is the ingress public key---i.e. the key that senders should encrypt their fog hint for). When we switch to DCAP, I'd like to change this to some kind of a MAC over an associated data structure that contains these keys (i.e. give us more than 64 bytes worth of data that we can attest to), but that's not how it works now.

The upshot is that right now, mc-attest-ake needs to be able to check that the first 32 bytes match, and ignore the rest of ReportData.

@nick-mobilecoin
Copy link
Collaborator Author

created #28 to address the partial verification

@nick-mobilecoin
Copy link
Collaborator Author

@samdealy bumped for re-review due to argument docs cleanup

Base automatically changed from nick/mr_signer to main March 4, 2023 04:16
Copy link

@NotGyro NotGyro left a comment

Choose a reason for hiding this comment

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

Looks good to me, especially now that #27 is noted and won't get forgotten

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Medium-sized PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants