-
Notifications
You must be signed in to change notification settings - Fork 204
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
[tools,tests] Decouple encrypted file tools from SGX #2080
[tools,tests] Decouple encrypted file tools from SGX #2080
Conversation
586af5d
to
60f341b
Compare
dac67e1
to
1547085
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 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)
a discussion (no related file):
@woju: Plz review this PR, it touches packaging (only a bit though).
Suggestion:
Decouple encrypted files tools from SGX
-- commits
line 9 at r1:
ditto x2
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 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
@woju: Plz review this PR, it touches packaging (only a bit though).
Sure.
debian/gramine.install
line 13 at r1 (raw file):
usr/lib/${DEB_HOST_MULTIARCH}/libcbor.a* usr/lib/${DEB_HOST_MULTIARCH}/libmbed*_gramine.* usr/lib/${DEB_HOST_MULTIARCH}/libpf_util.a*
Why do you want to ship this library? Who will be using it and where? Where are headers matching this library packaged?
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, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow and @woju)
debian/gramine.install
line 13 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Why do you want to ship this library? Who will be using it and where? Where are headers matching this library packaged?
Good questions.
I simply kept the same pattern we use for shipping libsgx_util.a
:
gramine/tools/sgx/common/meson.build
Lines 23 to 24 in 21ae038
install: true, | |
install_rpath: get_option('prefix') / get_option('libdir'), |
libsgx_util
. I can't recall the exact reason, but it might be a legacy decision from when we built libsgx_util
as a shared library, which was linked to several libraries and tools. Now that we link all dependencies of our tools/sgx
statically: 4e11bb9, we might not need to ship it at all.
What if we don't ship either of them?
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, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @mkow)
debian/gramine.install
line 13 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Good questions.
I simply kept the same pattern we use for shipping
libsgx_util.a
:. However, I'm reconsidering why we even need to shipgramine/tools/sgx/common/meson.build
Lines 23 to 24 in 21ae038
install: true, install_rpath: get_option('prefix') / get_option('libdir'), libsgx_util
. I can't recall the exact reason, but it might be a legacy decision from when we builtlibsgx_util
as a shared library, which was linked to several libraries and tools. Now that we link all dependencies of ourtools/sgx
statically: 4e11bb9, we might not need to ship it at all.What if we don't ship either of them?
Unless you have external consumers for libpf_util
, we shouldn't ship it (nor its headers).
libsgx_util
however is used in contrib, for ITA and MAA integrations:
- https://github.com/gramineproject/contrib/blob/5902e475bcb05b05ba5ae2e7e7a881a9e56e63e0/Integrations/intel/ra_tls_ita/meson.build#L66
- https://github.com/gramineproject/contrib/blob/5902e475bcb05b05ba5ae2e7e7a881a9e56e63e0/Integrations/azure/ra_tls_maa/meson.build#L66
and I think we ship headers for it. So we can't just stop shipping libsgx_util
.
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: 14 of 19 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @woju)
Previously, mkow (Michał Kowalczyk) wrote…
ditto x2
Will fix during rebase.
debian/gramine.install
line 13 at r1 (raw file):
libsgx_util
however is used in contrib, for ITA and MAA integration
Ah, thanks for reminding! Kept libsgx_util
and removed libpf_util
.
-- commits
line 2 at r1:
Will fix during rebase.
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 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
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 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)
Encrypted files are not SGX-only and the related tools (`gramine-sgx-pf-crypt`, `gramine-sgx-pf-tamper`) are no longer part of the Linux-SGX PAL. This commit decouples the encrypted files tools from `tools/sgx` and removes SGX dependency for encrypted files tests. Signed-off-by: Kailun Qin <[email protected]>
b42eb2c
to
3cb70dd
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mkow)
Previously, kailun-qin (Kailun Qin) wrote…
Will fix during rebase.
Done.
Previously, kailun-qin (Kailun Qin) wrote…
Will fix during rebase.
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.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Description of the changes
Encrypted files are not SGX-only and the related tools (
gramine-sgx-pf-crypt
,gramine-sgx-pf-tamper
) are no longer part of the Linux-SGX PAL.This commit decouples the encrypted file tools from
tools/sgx
and removes SGX dependency for encrypted file tests.How to test this PR?
CI.
This change is