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

Implement Message for EvidenceKind #3605

Merged
merged 3 commits into from
Oct 6, 2023
Merged

Conversation

nick-mobilecoin
Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin commented Oct 5, 2023

Previously EvidenceMessage was a proxy to decode the EvidenceKind
enum. This resulted in clients needing to first attempt to decode as
EvidenceMessage and then fall back to VerificationReport. Now the
logic of decoding VerificationReport versus DcapEvidence is wrapped
in the Message implementation of EvidenceKind.

Previously `EvidenceMessage` was a proxy to decode the `EvidenceKind`
enum. This resulted in clients needing to first attempt to decode as
`EvidenceMessage` and then fall back to `VerificationReport`. Now the
logic of decoding `VerificationReport` versus `DcapEvidence` is wrapped
in the `Message` implementation of `EvidenceKind`.
@nick-mobilecoin
Copy link
Collaborator Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

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.

I don't think I have a super clear understanding of how this gets used.
How does this look in a .proto file or non-Rust code that wants to look at the report?

attest/verifier/types/src/verification.rs Outdated Show resolved Hide resolved
Use constant of `TAG_DCAP_EVIDENCE` instead of the number `4` directly

Co-authored-by: Eran Rundstein <[email protected]>
@nick-mobilecoin
Copy link
Collaborator Author

I don't think I have a super clear understanding of how this gets used. How does this look in a .proto file or non-Rust code that wants to look at the report?

Sorry, that context wasn't really provided.
This wouldn't work in a .proto file. Fortunately all places where the VerificationReport leaves a service via protobufs it's a field inside of another type so we're able to leverage oneof in those cases.

Where this proposed change comes into play, is in 2 places:

  1. Sending the VerificationReport around in the kex handshake,
  1. Watcher being able to retrieve previously stored VerificationReport and store new DcapEvidence unambiguosly

The first use case is temporary and will only need to persist for clients to ease the transition from VerificationReport to DcapEvidence.

For watcher, I think it will need to persist indefinitely. It may be that we could migrate the entry in the database similar to what Fog Report is doing,

let report_bytes =
.
Fog report didn't need special logic as it's an ephemeral entry that will be changed on service restart.

@eranrund
Copy link
Contributor

eranrund commented Oct 6, 2023

I don't think I have a super clear understanding of how this gets used. How does this look in a .proto file or non-Rust code that wants to look at the report?

Sorry, that context wasn't really provided. This wouldn't work in a .proto file. Fortunately all places where the VerificationReport leaves a service via protobufs it's a field inside of another type so we're able to leverage oneof in those cases.

Where this proposed change comes into play, is in 2 places:

1. Sending the `VerificationReport` around in the kex handshake,


* https://github.com/mobilecoinfoundation/mobilecoin/blob/f48e2267a4f269097d4459ab17b874e021250171/attest/ake/src/responder.rs#L93

* https://github.com/mobilecoinfoundation/mobilecoin/blob/f48e2267a4f269097d4459ab17b874e021250171/attest/ake/src/initiator.rs#L163

* https://github.com/mobilecoinfoundation/mobilecoin/blob/f48e2267a4f269097d4459ab17b874e021250171/attest/ake/src/initiator.rs#L222


2. Watcher being able to retrieve previously stored `VerificationReport` and store new `DcapEvidence` unambiguosly


* https://github.com/mobilecoinfoundation/mobilecoin/blob/f48e2267a4f269097d4459ab17b874e021250171/watcher/src/watcher_db.rs#L722

* https://github.com/mobilecoinfoundation/mobilecoin/blob/f48e2267a4f269097d4459ab17b874e021250171/watcher/src/watcher_db.rs#L786

The first use case is temporary and will only need to persist for clients to ease the transition from VerificationReport to DcapEvidence.

For watcher, I think it will need to persist indefinitely. It may be that we could migrate the entry in the database similar to what Fog Report is doing,

let report_bytes =

.
Fog report didn't need special logic as it's an ephemeral entry that will be changed on service restart.

Thanks for the detailed explanation! I like the thought of not having this kind of hack stick around forever, so I think a migration for the watcher to use a oneof field would be beneficial.

Previously `Message` was implemented for `EvidenceKind`. In order to
reduce the possibility that someone tries to use `EvidenceKind` directly
in a protobuf message, the encoding and decoding between
`VerificationReport` and `DcapEvidence` has been moved to dedicated
methods on the `EvidenceKind`.
@nick-mobilecoin nick-mobilecoin enabled auto-merge (squash) October 6, 2023 19:49
@nick-mobilecoin nick-mobilecoin merged commit 9fbaa76 into master Oct 6, 2023
20 checks passed
@nick-mobilecoin nick-mobilecoin deleted the nick/evidence-decode branch October 6, 2023 20:01
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.

2 participants