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 ISV product ID and ISV SVN fields to manifest #502

Merged
merged 1 commit into from Oct 7, 2022
Merged

Add ISV product ID and ISV SVN fields to manifest #502

merged 1 commit into from Oct 7, 2022

Conversation

DL8
Copy link
Contributor

@DL8 DL8 commented Sep 8, 2022

ISV product ID and ISV SVN fields are defined and signed in SIGSTRUCT. In Gramine, they are defined in the manifest and then used on enclave signing in enclave build flow. Add the option to configure them in OpenFL manifest with new flags to the makefile: SGX_ISVPRODID and SGX_ISVSVN. Their default values are 0, to maintain backwards compatibility.

Currently ISV SVN is configurable in build time, but this is not necessarily the desired behavior. Required behavior is open for discussion and should be updated once a decision is made.

Related issue: #501

ISV product ID and ISV SVN fields are defined and signed in SIGSTRUCT.
In Gramine, they are defined in the manifest and then used on enclave
signing in enclave build flow. Add the option to configure them in
OpenFL manifest with new flags to the makefile: SGX_ISVPRODID and
SGX_ISVSVN. Their default values are 0, to maintain backwards
compatibility.

Currently ISV SVN is configurable in build time, but this is not
necessarily the desired behavior. Required behavior is open for
discussion and should be updated once a decision is made.
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@DL8
Copy link
Contributor Author

DL8 commented Sep 8, 2022

I have read the CLA Document and I hereby sign the CLA

@DL8
Copy link
Contributor Author

DL8 commented Sep 8, 2022

recheck

github-actions bot added a commit that referenced this pull request Sep 8, 2022
@igor-davidyuk
Copy link
Contributor

igor-davidyuk commented Sep 18, 2022

Thank you for your contribution!
I must note that no example in https://github.com/gramineproject/examples feature these fields in manifests and the related section in Gramine docs is rather humble. Thus it is unclear how developers can come up with relevant product ID numbers and automate manifest versioning.

@DL8
Copy link
Contributor Author

DL8 commented Sep 18, 2022

Thank you for your contribution! I must note that no example in https://github.com/gramineproject/examples feature these fields in manifests and the related section in Gramine docs is rather humble. Thus it is unclear how developers can come up with relevant product ID numbers and automate manifest versioning.

I guess the ISV product ID and SVN are not an important detail in Gramine examples, so zero is used implicitly. They are mentioned in the samples of Intel's SGX SDK, but I looked at some of them and they used zeroes.

More information about those fields can be found here. Quote (emphasis is mine):

Each enclave is associated with a SIGSTRUCT structure, which is signed by its author and contains the enclave measure, signer public key, version number (ISV, reflecting the security level) and product identifier (ISVPRODID, to distinguish between enclaves from the same author). It allows to ensure that the enclave hasn't been modified and then re-signed with a different key.

As I understand it, product ID convention is up to enclave developer. Assigning each enclave with a different number (e.g. 1 for server and 2 for client) is a good approach. Once chosen, I don't see any reason to change it.
I don't understand what is manifest versioning. Do you mean ISVSVN? This is not manifest version, but enclave security version. It should start with zero and increment when a security vulnerability is fixed (not 100% automatic).

@igor-davidyuk
Copy link
Contributor

Thank you for the detailed explanation, @DL8
We can start incrementing ISVSVN on our side. Another idea is to bind this number to the OpenFL version, as it is the main application running inside our enclave, do you think it is a good idea?

@DL8
Copy link
Contributor Author

DL8 commented Sep 20, 2022

@igor-davidyuk you shouldn't increment ISVSVN with every version, but only in case the new version includes security vulnerability fixes. Also, if the SVN was incremented, it should be mentioned in the release notes

@igor-davidyuk
Copy link
Contributor

Ok then, seems like there is nothing to add to this PR. Are you going to remove WIP?

@igor-davidyuk igor-davidyuk self-assigned this Sep 20, 2022
@igor-davidyuk igor-davidyuk added the enhancement New feature or request label Sep 20, 2022
@DL8
Copy link
Contributor Author

DL8 commented Sep 28, 2022

If you believe this is the right way to implement this, I will remove the WIP

@DL8 DL8 changed the title WIP: Add ISV product ID and ISV SVN fields to manifest Add ISV product ID and ISV SVN fields to manifest Sep 28, 2022
Copy link
Contributor

@igor-davidyuk igor-davidyuk left a comment

Choose a reason for hiding this comment

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

A good first approach!

@psfoley psfoley merged commit 928efdc into securefederatedai:develop Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants