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 wrapping container for Quote3 and TcbInfoRaw #99

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

nick-mobilecoin
Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin commented May 26, 2023

Add a new wrapping container, Evidence that takes a Quote3 and a
TcbInfoRaw. The Evidence can be used with the majority of the
Verifier implementations. This allows one to compose one verifier and
use the Evidence in the verify() method.

Motivation

When working on an example for the verifiers it felt like there was quite a bit of work clients needed to do to get things right. The intent is to make it less error prone and easier for clients to work with.

This change may likely need some discussion about if this path is the correct way to go.

@nick-mobilecoin
Copy link
Collaborator Author

nick-mobilecoin commented May 26, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions github-actions bot added size/L Large PRs rust Pull requests that update rust code labels May 26, 2023
@meowblecoinbot meowblecoinbot requested a review from a team May 26, 2023 16:26
@github-actions
Copy link

github-actions bot commented May 26, 2023

❌ Unreviewed dependencies found

Crate Version Reviews (N/2) LoC Left-Pad Index Geiger Flags
yare 1.0.2 0 89 11 0 ____
mc-sgx-dcap-sys-types 0.6.1 0 97 3 0 CB__
mc-sgx-core-sys-types 0.6.1 0 224 5 0 CB__
subtle 2.5.0 0 440 146 9 ____
displaydoc 0.2.4 0 600 58 20 ____
x509-cert 0.2.3 0 1963 6 0 ____
mc-sgx-dcap-types 0.6.1 0 1990 0 0 ____
mc-sgx-core-types 0.6.1 0 2053 1 0 ____
textwrap 0.16.0 0 2234 74 0 ____
p256 0.13.2 0 2545 25 0 ____

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #99 (5b62625) into main (8413e9c) will increase coverage by 0.49%.
The diff coverage is 97.89%.

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
+ Coverage   97.90%   98.39%   +0.49%     
==========================================
  Files           7        9       +2     
  Lines        2101     2311     +210     
==========================================
+ Hits         2057     2274     +217     
+ Misses         44       37       -7     
Impacted Files Coverage Δ
verifier/src/lib.rs 96.22% <ø> (+3.49%) ⬆️
verifier/src/error.rs 70.00% <70.00%> (ø)
verifier/src/evidence.rs 99.09% <99.09%> (ø)
verifier/src/advisories.rs 99.11% <100.00%> (-0.04%) ⬇️
verifier/src/tcb.rs 99.81% <100.00%> (-0.01%) ⬇️

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

@nick-mobilecoin nick-mobilecoin requested review from eranrund and cbeck88 and removed request for a team May 26, 2023 16:59
Copy link
Contributor

@eranrund eranrund left a comment

Choose a reason for hiding this comment

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

Code-wise this looks fine to me. I like the verbose tree display thing and I like having an example output in the tests.
I don't feel like I can meaningfully comment on the quote types due to my lack of familiarity with the DCAP data types... but everything appears well documented and fairly easy to follow so I am okay with it getting merged if @cbeck88 agrees with that.

Having to manually construct that And<...> type is indeed suboptimal but I also couldn't figure out a way to avoid it due to wanting to use multiple traits on that thing. So returning an impl Verifier<...> or a Box<dyn Verifier...> doesn't seem to work.

How would this look in clients? Would they have to construct all this And<> stuff or is there going to be some standard verifier of some sort that we use in all our clients?

verifier/src/evidence.rs Show resolved Hide resolved
@nick-mobilecoin
Copy link
Collaborator Author

How would this look in clients? Would they have to construct all this And<> stuff or is there going to be some standard verifier of some sort that we use in all our clients?

That is something I'm still unsure of.
I'm thinking we might provide common MRSIGNER and MRENCLAVE verifier type, that would still require clients to build in the correct order, or we take in a series of arguments.
I really wish I could figure out a way to use Box or dyn here, but I was unable to display the error tree with them.

@eranrund
Copy link
Contributor

How would this look in clients? Would they have to construct all this And<> stuff or is there going to be some standard verifier of some sort that we use in all our clients?

That is something I'm still unsure of. I'm thinking we might provide common MRSIGNER and MRENCLAVE verifier type, that would still require clients to build in the correct order, or we take in a series of arguments. I really wish I could figure out a way to use Box or dyn here, but I was unable to display the error tree with them.

That might be possible with another trait that inherits the Verifier and tree display traits, and has a blanket implementation for everything that implements these two. Kinda hacky...

Add a new wrapping container, `Evidence` that takes a `Quote3` and a
`TcbInfoRaw`. The `Evidence` can be used with the majority of the
`Verifier` implementations. This allows one to compose one verifier and
use the `Evidence` in the `verify()` method.
@nick-mobilecoin nick-mobilecoin merged commit c411bb0 into main Jun 23, 2023
24 of 25 checks passed
@nick-mobilecoin nick-mobilecoin deleted the nick/consolidated-evidence branch June 23, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update rust code size/L Large PRs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants