-
Notifications
You must be signed in to change notification settings - Fork 201
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
remove launch token dependency for DCAP enclaves #363
remove launch token dependency for DCAP enclaves #363
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 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @lead4good)
a discussion (no related file):
@lead4good You are absolutely correct: tokens (files containing EINITTOKEN) are only needed for legacy non-FLC (Flexible Launch Control) SGX hardware. In the newer SGX hardware, FLC feature was added (which also coincides with the introduction of the DCAP driver and attestation mode).
Gramine strives to support both old and new SGX hardware, that's why we still rely on tokens (generate them, even if they are dummy).
Why do we always generate and read tokens in Gramine, even though they are not needed in DCAP SGX environments (and thus are mocked via a dummy token)? This is because we want to keep consistency of Gramine usage by higher-level tools and flows. One good example is GSC: https://github.com/gramineproject/gsc. GSC uses Gramine and its tools under the hood, and it would be very cumbersome to have two paths (e.g., two sets of GSC scripts): one for EPID with real tokens, and one for DCAP without tokens.
TLDR: By having dummy tokens, we make our lives easier -- tools/scripts that use Gramine can use the exact same paths/flows for both EPID and DCAP environments.
@lead4good Is there a particular reason you don't want to have this dummy token? Other than "striving for minimality"?
a discussion (no related file):
The PR is generally correct but it doesn't go far enough: we could also remove some code from our Python tools (gramine-sgx-get-token
as one example). What this PR is it removes the need to have the token file laying around when Gramine starts a workload. But the build process is unchanged, and thus Gramine will still generate a redundant token file. (Redundant for DCAP environments, I mean.)
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.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@lead4good You are absolutely correct: tokens (files containing EINITTOKEN) are only needed for legacy non-FLC (Flexible Launch Control) SGX hardware. In the newer SGX hardware, FLC feature was added (which also coincides with the introduction of the DCAP driver and attestation mode).
Gramine strives to support both old and new SGX hardware, that's why we still rely on tokens (generate them, even if they are dummy).
Why do we always generate and read tokens in Gramine, even though they are not needed in DCAP SGX environments (and thus are mocked via a dummy token)? This is because we want to keep consistency of Gramine usage by higher-level tools and flows. One good example is GSC: https://github.com/gramineproject/gsc. GSC uses Gramine and its tools under the hood, and it would be very cumbersome to have two paths (e.g., two sets of GSC scripts): one for EPID with real tokens, and one for DCAP without tokens.
TLDR: By having dummy tokens, we make our lives easier -- tools/scripts that use Gramine can use the exact same paths/flows for both EPID and DCAP environments.
@lead4good Is there a particular reason you don't want to have this dummy token? Other than "striving for minimality"?
First and foremost the reason I supplied this patch is to remove the python runtime dependency (for DCAP, I only use DCAP, who uses EPID today?). There was a brief discussion here: gramineproject/gsc#37 which also mentions the requirement to create tokens on the executing machine. I see no reason to restrict a user in such ways if it is not necessary.
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The PR is generally correct but it doesn't go far enough: we could also remove some code from our Python tools (
gramine-sgx-get-token
as one example). What this PR is it removes the need to have the token file laying around when Gramine starts a workload. But the build process is unchanged, and thus Gramine will still generate a redundant token file. (Redundant for DCAP environments, I mean.)
I have removed the dummy function and added a hint for anyone using the get token tool.
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 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @lead4good)
a discussion (no related file):
for DCAP, I only use DCAP, who uses EPID today?
Yeah, it's generally true. EPID is slowly fading away. Our (Gramine devs) lifes would be much easier if EPID wouldn't exist at all :)
to remove the python runtime dependency
This is a very good reason. I didn't think of it that way. Do you know how much you win by this, not installing Python on the deployment machine/VM/Docker image? Just curious.
a discussion (no related file):
Previously, lead4good wrote…
I have removed the dummy function and added a hint for anyone using the get token tool.
Yep, this looks good.
a discussion (no related file):
Since we're currently busy with other stuff (preparing for the new release), I suggest to put this PR on hold and then discuss what to do with it.
python/graminelibos/sgx_get_token.py, line 139 at r2 (raw file):
'environments. To keep consistency of Gramine usage by higher-level tools and' 'flows this tool will output an empty dummy token which can - but does not need to' 'be - supplied during launch.' )
I'll need to come up with a suggestion for a better phrasing. Keeping a blocking comment until I suggest some text.
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.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @lead4good)
a discussion (no related file):
Do you know how much you win by this, not installing Python on the deployment machine/VM/Docker image?
No, I haven't checked yet. There are various other benefits though: Pycache files are an annoying source of non determinism, python related files don't clog the manifest file and finally we could now have a look at removing python as a "sign" dependency. Ifgramine-sgx-sign
wouldn't depend on python, at least the buildpack that I'm working on, but maybe also GSC could drop python completely which makes them much less complex and much easier to port to different base images.
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Since we're currently busy with other stuff (preparing for the new release), I suggest to put this PR on hold and then discuss what to do with it.
Done.
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.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
python/graminelibos/sgx_get_token.py, line 139 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'll need to come up with a suggestion for a better phrasing. Keeping a blocking comment until I suggest some text.
Can this also be under if verbose
? For example, gramine-test --sgx build
will generate hundreds of these warnings.
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.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, lead4good wrote…
Do you know how much you win by this, not installing Python on the deployment machine/VM/Docker image?
No, I haven't checked yet. There are various other benefits though: Pycache files are an annoying source of non determinism, python related files don't clog the manifest file and finally we could now have a look at removing python as a "sign" dependency. Ifgramine-sgx-sign
wouldn't depend on python, at least the buildpack that I'm working on, but maybe also GSC could drop python completely which makes them much less complex and much easier to port to different base images.
Thanks @lead4good for a detailed answer.
I like your idea of removing the Python dependency completely from Gramine (from the enclave building steps in Gramine), but I think this is hardly possible. Yes, we can remove Python dependencies for enclave running in Gramine, which is already a big win. But a lot of building utilities/flows are based on Python, and Gramine is actually moving more and more tools to Python.
a discussion (no related file):
Previously, lead4good wrote…
Done.
The release (v1.2) was done last week, so we can revive this PR. Since @lead4good is not working on Gramine any more (I assume?), I will take over this PR and continue its development.
The SGX Launch Token (aka EINITTOKEN) file is required for EPID-based (more specifically, for non-FLC-based) SGX platforms. On these platforms, the token file is generated by the Quoting Enclave (QE) right-before the startup of the application in Gramine (Gramine sends a request for generation of the token to QE via the AESM service, using the `gramine-sgx-get-token` Python tool). Later, during enclave initialization at Gramine startup, this token file is read and its contents are provided as an arg to `SGX_IOC_ENCLAVE_INIT` ioctl. However, this token file is not required for DCAP-based (more specifically, for FLC-based) SGX platforms. Previously, Gramine still required to use the `gramine-sgx-get-token` Python tool even on these platforms, which generated a dummy token file. Generating this dummy token file may be problematic: (a) this cannot work on read-only FS mounts, and (b) it requires Python installed on the system. So this commit removes this dummy token file completely on DCAP machines. Co-authored-by: Dmitrii Kuvaiskii <[email protected]> Signed-off-by: Frieder Paape, Integritee AG <[email protected]> Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
cf3af81
to
e6fe602
Compare
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 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
a discussion (no related file):
I took over this PR, rebased it to the latest master and fixed some stuff. Please review the whole PR, not the latest diff (because of the rebase, it will be painful).
Pal/src/host/Linux-SGX/sgx_main.c
line 217 at r3 (raw file):
sgx_arch_token_t enclave_token; char* token_path = NULL; int token_fd = -1;
FYI: This rearrangement of variable declarations is purely cosmetic. Reads better to me.
python/graminelibos/sgx_get_token.py
line 139 at r2 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Can this also be under
if verbose
? For example,gramine-test --sgx build
will generate hundreds of these warnings.
Done. Also fixed the comment.
Jenkins, test this please |
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
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 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners
Pal/src/host/Linux-SGX/sgx_framework.c
line 122 at r4 (raw file):
/* DCAP platforms do not use SGX tokens so we get enclave attributes from SIGSTRUCT instead, * but EPID platforms require SECS to be populated from the SGX token */
FYI: On my previous try, the Jenkins CI (which uses EPID) failed all SGX tests with error: enclave EINIT failed - Invalid enclave attribute
.
To be honest, I don't understand what exactly is bad about taking attributes
/misc_select
from SIGSTRUCT instead of the EINITTOKEN, but whatever, I added this distinction in the fixup commit. I don't want to debug this EPID craziness.
Jenkins, test this please |
Closing this PR as it is considered as a stop-gap solution, and also it is not high priority. We better fix it properly. Some context. This PR and gramineproject/gsc#62 allow Gramine to ignore The solution can be considered partial. Another proposal is more radical but also more correct: add a C/Rust version of |
@dimakuv From my side it is fine to close this and work on a proper fix, as you suggest. For us it doesn't have very high priority as there is an easy work around. For DCAP environments, we just run |
AFAICS, this PR could have re-purposed to that direction already: it implements the "when needed" logic and the DCAP part would have been already covered. |
@mythi The huge missing piece in this PR is the re-write of the |
For the DCAP path it is, no? |
I see your point now. Yes, for DCAP it is enough. Still, it is better to submit a "full" PR that works both for DCAP and EPID. |
Looking into a C port for
gramine-sgx-get-token
to get rid of the python runtime dependency I discovered that DCAP enclaves do not need a real token and instead just depend on a dummy token. Looking further I found that the dummy token information is generated from the SIGSTRUCT, so instead of using the information from the dummy token struct I took it directly from the SIGSTRUCT.This removes the dummy launch token of DCAP enclaves and hence the python runtime dependency of DCAP enclaves.
See also:
Related PR in GSC repo: gramineproject/gsc#62
This change is