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

Change from passing verifier to passing measurements #3375

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

nick-mobilecoin
Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin commented Jun 2, 2023

Previously many interfaces that wished to attest to an enclave would
create and pass a verifier down to the logic that would eventually try
and verify the enclave's report.
Now these interfaces pass in a Vec<TrustedMeasurement> with all of the
measurements they wish to verify against. This isolates the logic for
how an enclave report is verified to support different verification
methods based on EPID vs DCAP.

@nick-mobilecoin
Copy link
Collaborator Author

nick-mobilecoin commented Jun 2, 2023

@nick-mobilecoin nick-mobilecoin self-assigned this Jun 2, 2023
@nick-mobilecoin nick-mobilecoin changed the base branch from nick/trusted-measurements to nick/foundation-verifier-to-trusted June 5, 2023 16:10
@nick-mobilecoin nick-mobilecoin force-pushed the nick/verifier-trusted-measurement branch from 1ccb67c to 196a0ec Compare June 5, 2023 16:10
@nick-mobilecoin nick-mobilecoin marked this pull request as ready for review June 5, 2023 16:10
@cbeck88
Copy link
Contributor

cbeck88 commented Jun 26, 2023

I find it a little funky that we pass Vec<TrustedMeasurements> rather than just TrustedMeasurements ? This is already a collection, right?

@cbeck88
Copy link
Contributor

cbeck88 commented Jun 26, 2023

I feel like it might be simpler to pass just TrustedMeasurements, but fine either way

@nick-mobilecoin
Copy link
Collaborator Author

I find it a little funky that we pass Vec<TrustedMeasurements> rather than just TrustedMeasurements ? This is already a collection, right?

TrustedMeasurement is one measurement, a MrEnclave or a MrSigner version, not a collection.

@cbeck88
Copy link
Contributor

cbeck88 commented Jun 26, 2023

I see, I got confused and thought it was this:

pub struct TrustedMeasurementSet {

@cbeck88
Copy link
Contributor

cbeck88 commented Jun 26, 2023

I think one concern i have with passing sets of measurements around is that different advisories may apply to them but there will be no way to express that in the api. Part of the motivation for the attest verifier config crate

@cbeck88 cbeck88 closed this Jun 26, 2023
@cbeck88 cbeck88 reopened this Jun 26, 2023
@cbeck88
Copy link
Contributor

cbeck88 commented Jun 26, 2023

Misclick sorry

@cbeck88
Copy link
Contributor

cbeck88 commented Jun 26, 2023

Part of the motivation for the attest verifier config crate was that SW hardening advisories may be fixed in newer enclave versions but not in historical ones, so apis that just pass lists of measurements around without tracking that are kind of fucked -- there's no way to capture that.

I know that's the status quo right now, but James blocked earlier PRs that i made that were along the lines of what you are doing now

Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

Lets pass TrustedMeasurementSet or a list of StatusVerifierConfig or something that associates hardening advisories to individual MRENCLAVE etc.

@nick-mobilecoin
Copy link
Collaborator Author

Lets pass TrustedMeasurementSet or a list of StatusVerifierConfig or something that associates hardening advisories to individual MRENCLAVE etc.

It may be a naming thing with with the ambiguity of "measurement"
In this PR the TrustedMrEnclaveMeasurement and the TrustedMrSignerMeasurement includes the advisories:

pub struct TrustedMrEnclaveMeasurement {
    /// The MRENCLAVE measurement
    ///
    /// For JSON this will be hex-encoded bytes.
    #[serde(with = "hex", rename = "MRENCLAVE")]
    mr_enclave: [u8; 32],
    /// The list of config advisories that are known to be mitigated in
    /// software at this enclave revision.
    #[serde(default)]
    mitigated_config_advisories: Vec<String>,
    /// The list of hardening advisories that are known to be mitigated in
    /// software at this enclave revision.
    #[serde(default)]
    mitigated_hardening_advisories: Vec<String>,
}

@cbeck88
Copy link
Contributor

cbeck88 commented Jun 26, 2023

Okay, i get it now, thanks

@nick-mobilecoin
Copy link
Collaborator Author

I wonder what a good name would for these
"measurement" was already a bit overloaded as it's use in the SGX SDK only captured the mrsigner key value and not the ISVSVN and product ID.

Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

Gonna go drink my coffee 😮‍💨 😁☕

@nick-mobilecoin nick-mobilecoin force-pushed the nick/foundation-verifier-to-trusted branch from bb606ce to 732810a Compare June 26, 2023 20:37
@nick-mobilecoin nick-mobilecoin force-pushed the nick/verifier-trusted-measurement branch from 21f3ded to 96ec7cd Compare June 26, 2023 20:37
Base automatically changed from nick/foundation-verifier-to-trusted to master July 10, 2023 15:10
@nick-mobilecoin nick-mobilecoin force-pushed the nick/verifier-trusted-measurement branch from 96ec7cd to f645f08 Compare July 11, 2023 23:44
@github-actions
Copy link

❌ Heads up, I tried building android-bindings using this branch and it failed.
Build logs can be found by inspecting the github action run Check that repositories submoduling us will still build after this PR / android-bindings or by clicking here.

@github-actions
Copy link

❌ Heads up, I tried building full-service using this branch and it failed.
Build logs can be found by inspecting the github action run Check that repositories submoduling us will still build after this PR / full-service or by clicking here.

Previously many interfaces that wished to attest to an enclave would
create and pass a verifier down to the logic that would eventually try
and verify the enclave's report.
Now these interfaces pass in a `Vec<TrustedMeasurement>` with all of the
measurements they wish to verify against. This isolates the logic for
how an enclave report is verified to support different verification
methods based on EPID vs DCAP.
@nick-mobilecoin
Copy link
Collaborator Author

Kicking for re-review since I was waiting for the mc-attestation-verifier crate to come in and this resulted in more than minor changes once it rebased.

connection/Cargo.toml Outdated Show resolved Hide resolved
fog/report/cli/src/main.rs Outdated Show resolved Hide resolved
mobilecoind/src/bin/main.rs Outdated Show resolved Hide resolved
The debug messages specifying the MRSIGNER identities now say which
enclave they are for.
@nick-mobilecoin nick-mobilecoin merged commit a882be4 into master Jul 25, 2023
@nick-mobilecoin nick-mobilecoin deleted the nick/verifier-trusted-measurement branch July 25, 2023 14:54
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.

4 participants