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.

Sign up for GitHub

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

Jump to bottom

feat: configuration for attestation #107

Merged
merged 1 commit into from
Dec 6, 2022
Merged

feat: configuration for attestation #107

merged 1 commit into from
Dec 6, 2022

Conversation

rjzak
Copy link
Member

@rjzak rjzak commented Nov 9, 2022 •
edited
Loading

Working on the configuration file, which:

  • Uses serde
  • Has the fields as Option, at least for now
  • Reads in hash values as a string, but converts to int array where needed.

Currently has src/main.rs in the attestation creates. These are convenience applications to make it easier to look at the CSRs I've collected instead of having to re-compile Steward and get them that way. They will be removed before approval.

CC: @npmccallum

Signed-off-by: Richard Zak [email protected]

@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from b88991c to 3bae6ca Compare November 9, 2022 20:22
@bstrie
Copy link
Contributor

bstrie commented Nov 10, 2022

Related: #1, #2, #75

@rjzak rjzak force-pushed the attestation_config branch 8 times, most recently from 9fbbf49 to 6e6a049 Compare November 11, 2022 20:48
@rjzak
Copy link
Member Author

rjzak commented Nov 11, 2022

Some recent changes beyond the config:

  • Renamed SNP's PlatformInfoFlags.SMT to PlatformInfoFlags.SME, and removed the FlagsSet component in PlatformInfoFlags since SME and TSME are mutually-exclusive. Doc
  • SNP verify() now checks that the Policy.abi_major and Policy.abi_minor match Report.current_major and Report.current_minor. The firmware should do this, but it doesn't hurt for Steward to also check.

@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from 554ee0f to 6627512 Compare November 11, 2022 22:16
@rjzak rjzak marked this pull request as ready for review November 11, 2022 22:17
@rjzak rjzak requested review from a team, bstrie and haraldh as code owners November 11, 2022 22:17
@rjzak rjzak force-pushed the attestation_config branch 7 times, most recently from e26d0d8 to 45d2937 Compare November 12, 2022 00:24
haraldh
haraldh requested changes Nov 14, 2022
View reviewed changes
crates/sgx_validation/src/config.rs Outdated Show resolved Hide resolved
crates/sgx_validation/src/config.rs Outdated Show resolved Hide resolved
crates/sgx_validation/src/config.rs Outdated Show resolved Hide resolved
crates/snp_validation/src/config.rs Outdated Show resolved Hide resolved
@haraldh
Copy link
Member

haraldh commented Nov 14, 2022

@npmccallum Would it make sense to create configs for different product ids?

haraldh
haraldh requested changes Nov 14, 2022
View reviewed changes
crates/sgx_validation/src/config.rs Outdated Show resolved Hide resolved
crates/snp_validation/src/config.rs Outdated Show resolved Hide resolved
crates/snp_validation/src/config.rs Outdated Show resolved Hide resolved
@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from 3d8f726 to bbe4a1f Compare November 14, 2022 16:56
rvolosatovs
rvolosatovs reviewed Dec 3, 2022
View reviewed changes
src/main.rs Outdated Show resolved Hide resolved
@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from 99c6039 to 8450b88 Compare December 4, 2022 05:06
@rjzak rjzak requested review from haraldh and rvolosatovs December 4, 2022 05:20
rvolosatovs
rvolosatovs reviewed Dec 4, 2022
View reviewed changes
crates/snp_validation/src/config.rs Outdated Show resolved Hide resolved
crates/validation_common/src/lib.rs Outdated Show resolved Hide resolved
crates/validation_common/src/lib.rs Outdated Show resolved Hide resolved
@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from 742e542 to 833ad1e Compare December 4, 2022 17:46
@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from 7d21b00 to 2c271b1 Compare December 5, 2022 21:34
@rjzak rjzak force-pushed the attestation_config branch from 2c271b1 to c70d441 Compare December 5, 2022 21:49
@dpal dpal added the attestation Issues related to attestation label Dec 6, 2022
@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from 4159161 to 3705f00 Compare December 6, 2022 16:24
This was referenced Dec 6, 2022
Open
Closed
@rjzak rjzak force-pushed the attestation_config branch 2 times, most recently from 16e8515 to c8b60d1 Compare December 6, 2022 17:39
rvolosatovs
rvolosatovs approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@rvolosatovs rvolosatovs left a comment

Choose a reason for hiding this comment

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

Good enough to begin iterative improvement process. Let's refine this as part of #138 and #139

@rjzak @rvolosatovs
feat: configuration for attestation
9a9d9ac 
Co-authored-by: Roman Volosatovs <[email protected]>
Signed-off-by: Richard Zak <[email protected]>
@rjzak rjzak force-pushed the attestation_config branch from c8b60d1 to 9a9d9ac Compare December 6, 2022 18:14
@rjzak rjzak enabled auto-merge (rebase) December 6, 2022 18:14
@rjzak rjzak dismissed haraldh’s stale review December 6, 2022 18:55

Harald and I met to discuss the outstanding items which were resolved via the following push

@rjzak rjzak merged commit 69a4f29 into enarx:main Dec 6, 2022
@rjzak rjzak deleted the attestation_config branch December 6, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvolosatovs rvolosatovs rvolosatovs approved these changes

@bstrie bstrie Awaiting requested review from bstrie

@haraldh haraldh Awaiting requested review from haraldh

Assignees

@rjzak rjzak

Labels
attestation Issues related to attestation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rjzak @bstrie @haraldh @rvolosatovs @dpal