-
Notifications
You must be signed in to change notification settings - Fork 210
Added KSS features support including Config ID/SVN #2101
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 13 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner
a discussion (no related file):
Known Issues
Currently, setting sgx.kss = false results in the following error:
gramine-sgx ./python scripts/sgx-quote.py Gramine is starting. Parsing TOML manifest file, this may take some time... error: Enclave creation IOCTL failed with Input/output error (EIO). Probably your manifest requires CPU features (e.g. `sgx.cpu_features.avx512 = "required"`) that are not available on this platform. error: Creating enclave failed: Input/output error (EIO) error: load_enclave() failed with error: Input/output error (EIO)
Since I was prioritizing the implementation of KSS support and considering that it would be best to merge the update before significant changes occur in the existing codebase, I have submitted this PR without fixing the above issue.
I would greatly appreciate any assistance in resolving this issue.
Moving into a blocking comment, this has to be fixed before merging.
Suggestion:
Add
-- commits
line 5 at r1:
This should be squashed together with the first commit, it's an integral part of it.
Suggestion:
Fix
CI-Examples/python/README.md
line 40 at r1 (raw file):
To set the Config ID and Config SVN, configure the following environment variables on the host OS before executing scripts:
I don't like that those are passed through envs, why not via cmdline arguments?
CI-Examples/python/README.md
line 40 at r1 (raw file):
To set the Config ID and Config SVN, configure the following environment variables on the host OS before executing scripts:
trailing spaces
python/graminelibos/manifest.py
line 310 at r1 (raw file):
sgx.setdefault('kss', False) sgx.setdefault('isvextprodid', "0x00000000000000000000000000000000") sgx.setdefault('isvfamilyid', "0x00000000000000000000000000000000")
Are those two values interpreted as numbers or byte arrays? I think it's the latter? Then they shouldn't have the hex-number prefix "0x".
pal/src/host/linux-sgx/host_main.c
line 204 at r1 (raw file):
static int initialize_enclave(struct pal_enclave* enclave, const char* manifest_to_measure, uint8_t *config_id, uint16_t config_svn) {
Please fix formatting and the code style in the whole PR (see our style guideline in the docs)
pal/src/host/linux-sgx/host_main.c
line 1135 at r1 (raw file):
} // convert hexstring to byte array
Please don't write comment which say exactly what the next line says. You have basically the same sentence in the function name, so this comment doesn't help in anything.
pal/src/host/linux-sgx/host_main.c
line 1136 at r1 (raw file):
// convert hexstring to byte array static int parse_hex_string_to_bytes(const char* hex_str, uint8_t* buffer, size_t buffer_size) {
We already have hex2bytes()
.
pal/src/host/linux-sgx/host_main.c
line 1147 at r1 (raw file):
buffer[i] = (uint8_t)strtol(byte_str, NULL, 16); } return 1;
Please use Gramine conventions, if you return status codes via an int
then 0 means success and negative means error.
pal/src/host/linux-sgx/host_main.c
line 1151 at r1 (raw file):
// convert hexstring to uint16_t static int parse_hex_string_to_uint16(const char* hex_str, uint16_t* value) {
We already have str_to_ulong()
, please use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner (waiting on @hello31337 and @mkow)
CI-Examples/python/README.md
line 40 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I don't like that those are passed through envs, why not via cmdline arguments?
+1
python/graminelibos/manifest.py
line 310 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Are those two values interpreted as numbers or byte arrays? I think it's the latter? Then they shouldn't have the hex-number prefix "0x".
+1
a discussion (no related file):
I think we also need to update gramine-sgx-sigstruct-view
and the associated manpage: https://github.com/gramineproject/gramine/blob/master/Documentation/manpages/gramine-sgx-sigstruct-view.rst.
a discussion (no related file):
Shouldn't we update here:
gramine/pal/src/host/linux-sgx/pal_process.c
Lines 108 to 115 in ef48c72
/* all Gramine enclaves with same config (manifest) should have same enclave information | |
* (in the future, we may exclude `config_svn` and `config_id` from this check -- they are set | |
* by the app enclave after EINIT and are thus not security-critical) */ | |
static_assert(offsetof(sgx_report_body_t, report_data) + sizeof(*expected_data) == | |
sizeof(sgx_report_body_t), "wrong sgx_report_body_t::report_data offset"); | |
if (memcmp(peer_enclave_info, &g_pal_linuxsgx_state.enclave_info, | |
offsetof(sgx_report_body_t, report_data))) | |
return false; |
a discussion (no related file):
I think we also need to update our RA-TLS lib (e.g., the verification callback), the corresponding examples and the attestation doc.
a discussion (no related file):
I think we should also note somewhere that setting config_svn
impacts the sealing key derivation:
gramine/pal/src/host/linux-sgx/enclave_framework.c
Lines 384 to 385 in ef48c72
memcpy(&key_request.config_svn, &g_pal_linuxsgx_state.enclave_info.config_svn, | |
sizeof(sgx_config_svn_t)); |
-- commits
line 4 at r1:
Pls add your sign-offs.
python/graminelibos/manifest_check.py
line 108 at r1 (raw file):
'kss': bool, 'isvfamilyid': str, 'isvextprodid': str,
These should be placed above the TODO.
Code quote:
'kss': bool,
'isvfamilyid': str,
'isvextprodid': str,
pal/src/host/linux-sgx/host_main.c
line 1293 at r1 (raw file):
if (strcmp(key, "SGX_CONFIG_ID") == 0) { if (!parse_hex_string_to_bytes(value, config_id, SGX_CONFIGID_SIZE)) { initialize_config_id(config_id);
I prefer that we fail loudly if parsing fails.
Code quote:
initialize_config_id(config_id);
pal/src/host/linux-sgx/host_main.c
line 1298 at r1 (raw file):
// parse SGX_CONFIG_SVN env to config_svn else if (strcmp(key, "SGX_CONFIG_SVN") == 0) { if (!parse_hex_string_to_uint16(value, &config_svn)) {
ditto
CI-Examples/python/python.manifest.template
line 19 at r1 (raw file):
loader.insecure__use_cmdline_argv = true sys.enable_sigterm_injection = false
Why this change?
Code quote:
false
CI-Examples/python/python.manifest.template
line 37 at r1 (raw file):
sys.enable_extra_runtime_domain_names_conf = true sgx.debug = false
ditto
Code quote:
false
CI-Examples/python/README.md
line 29 at r1 (raw file):
When KSS is enabled, you can set four values—ISV Family ID, ISV Extended Prod ID, Config ID, and Config SVN—and include them in the REPORT structure. To enable KSS, modify the following section in python.manifest.template:
trailing spaces
Code quote:
··
CI-Examples/python/README.md
line 34 at r1 (raw file):
To set the ISV Family ID and ISV Extended Prod ID, modify the following lines:
ditto
Code quote:
··
295f6dd
to
66cbaae
Compare
Signed-off-by: Ao Sakurai <[email protected]> [Docs] Added KSS-related descriptions to CI-Examples/python/README.md. Signed-off-by: Ao Sakurai <[email protected]>
Signed-off-by: Ao Sakurai <[email protected]>
Signed-off-by: Ao Sakurai <[email protected]>
66cbaae
to
e545a1e
Compare
Thank you very much for your review. Based on your comments, I have implemented a series of fixes and improvements. Some parts remain unaddressed; this is a deliberate choice we discussed in a recent meeting, where we agreed to prioritize responsiveness and delegate certain minor issues for future handling on the Gramine side. Below is a summary of the changes that have been applied, followed by items that are still pending. Please review at your convenience. Implemented Fixes and Improvements
Regarding the specification of Config ID and Config SVN: these values can now be provided either via command-line arguments or environment variables. Regardless of the method, the values are parsed in gramine.in and passed to host_main.c as command-line arguments. In other words, even when set via environment variables, they are internally parsed and forwarded as arguments. Regarding the callback mechanism: to support KSS without introducing breaking changes to existing products, a separate callback function was introduced with a modified interface. Remaining Items (Not Yet Addressed)
|
Description of the changes
In the existing PR#903, an update was introduced to allow setting the KSS (Key Separation and Sharing) enable flag, as well as the ISV Family ID and ISV Extended Prod ID, in the manifest when KSS is enabled.
However, PR#903 did not provide functionality to set the Config ID and Config SVN, which are also available for use with KSS. Additionally, it appears that PR#903 has not yet been merged as of now.
Therefore, based on the updated codebase provided in PR#903, I have implemented a complete KSS support update that also allows specifying Config ID and Config SVN.
Usage
The KSS enable flag, ISV Family ID, and ISV Extended Prod ID can be specified in the manifest in the same way as PR#903, as follows:
Config ID and Config SVN are implemented by setting environment variables on the host OS, transferring them to the PAL, and registering the received values in SECS before ECREATE.
Therefore, by setting the environment variables SGX_CONFIG_ID and SGX_CONFIG_SVN on the host OS in advance, you can specify the values you want to set.
You can verify these values by running sgx-report.py or sgx-quote.py in CI-Examples/python/scripts:
Known Issues
Currently, setting sgx.kss = false results in the following error:
Since I was prioritizing the implementation of KSS support and considering that it would be best to merge the update before significant changes occur in the existing codebase, I have submitted this PR without fixing the above issue.
I would greatly appreciate any assistance in resolving this issue.
How to test this PR?
This change is