-
Notifications
You must be signed in to change notification settings - Fork 193
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/sgx] Install RA-TLS and SecretProv libs properly #1114
Conversation
1036476
to
a5eb629
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: 0 of 19 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
a discussion (no related file):
The debian pipeline failed like this:
17:52:52 cp --reflink=auto -a debian/tmp/usr/lib/x86_64-linux-gnu/pkgconfig/mbedtls_gramine.pc debian/tmp/usr/lib/x86_64-linux-gnu/pkgconfig/ra_tls_gramine.pc debian/tmp/usr/lib/x86_64-linux-gnu/pkgconfig/secret_prov_gramine.pc debian/gramine//usr/lib/x86_64-linux-gnu/pkgconfig/
17:52:52 dh_install: error: missing files, aborting
17:52:52 install -d debian/.debhelper/generated/gramine
17:52:52 install -d debian/.debhelper/generated/gramine-ratls-dcap
17:52:52 install -d debian/.debhelper/generated/gramine-ratls-epid
17:52:52 make: *** [debian/rules:19: binary] Error 25
I cannot decipher this error, but looks like I have a mismatch with our Debian packages:
- I have
pkgconfig
forra_tls_gramine
(which has both attest and verifier sides, for both EPID and DCAP), andpkgconfig
forsecret_prov_gramine
(which has both attest and verifier sides, for both EPID and DCAP) - Our Debian packages are structured differently: both RA-TLS and SecretProv are considered as one, and the split happens on attest / verifier (the former goes into main
gramine
package, the latter goes intogramine-ratls-dcap
orgramine-ratls-epid
) and on DCAP / EPID.
@woju How to understand this Debian error, and how to consolidate our different views on splitting packages?
a5eb629
to
aec1ae9
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: 0 of 19 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The debian pipeline failed like this:
17:52:52 cp --reflink=auto -a debian/tmp/usr/lib/x86_64-linux-gnu/pkgconfig/mbedtls_gramine.pc debian/tmp/usr/lib/x86_64-linux-gnu/pkgconfig/ra_tls_gramine.pc debian/tmp/usr/lib/x86_64-linux-gnu/pkgconfig/secret_prov_gramine.pc debian/gramine//usr/lib/x86_64-linux-gnu/pkgconfig/ 17:52:52 dh_install: error: missing files, aborting 17:52:52 install -d debian/.debhelper/generated/gramine 17:52:52 install -d debian/.debhelper/generated/gramine-ratls-dcap 17:52:52 install -d debian/.debhelper/generated/gramine-ratls-epid 17:52:52 make: *** [debian/rules:19: binary] Error 25
I cannot decipher this error, but looks like I have a mismatch with our Debian packages:
- I have
pkgconfig
forra_tls_gramine
(which has both attest and verifier sides, for both EPID and DCAP), andpkgconfig
forsecret_prov_gramine
(which has both attest and verifier sides, for both EPID and DCAP)- Our Debian packages are structured differently: both RA-TLS and SecretProv are considered as one, and the split happens on attest / verifier (the former goes into main
gramine
package, the latter goes intogramine-ratls-dcap
orgramine-ratls-epid
) and on DCAP / EPID.@woju How to understand this Debian error, and how to consolidate our different views on splitting packages?
I don't know what happened in debian pipeline. Is it still happening?
WRT package split: if you want to split packages in some other way that would make more sense, by all means please do. The only thing that needs to be kept is that gramine
package needs to install without other repos (so the dependencies against packages in other repos need to be at most Recommends:
somewhere along the path).
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: 0 of 20 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
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
I don't know what happened in debian pipeline. Is it still happening?
WRT package split: if you want to split packages in some other way that would make more sense, by all means please do. The only thing that needs to be kept is that
gramine
package needs to install without other repos (so the dependencies against packages in other repos need to be at mostRecommends:
somewhere along the path).
Ok, I fixed all issues with packaging.
WRT package split: I figured I don't need to change the package split, because the new static libraries I introduce in this PR (libra_tls_verify.a
and libsecret_prov_verify.a
) should go into gramine
(these libraries are EPID- or DCAP-attestation agnostic, so should be in the general package).
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 19 files at r1.
Reviewable status: 1 of 20 files reviewed, 2 unresolved discussions, 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 (waiting on @dimakuv)
tools/sgx/ra-tls/ra_tls_common.h
line 6 at r3 (raw file):
/* * Internal RA-TLS details, to be used by external RA-TLS implementations: * - external implementation must implement ra_tls_verify_callback(),
external verification implementation? Or we actually don't consider ra_tls_attest
as external RA-TLS implementation?
Code quote:
external implementation
tools/sgx/ra-tls/secret_prov_common.h
line 5 at r3 (raw file):
/* * Internal Secret Prov details, to be used by external Secret Prov implementations:
What do we mean by external Secret Prov implementations
here? Do we expect users to have their own implementations of Secret Prov (even providing different user APIs
)?
Then it's a bit confusing here - in the current version of PR, it's secret_prov_verify
(incl. both secret_prov_common
and secret_prov_verify
) we actually supplied and we should expect this to be used for external implementation (e.g., just different in the underlying attestation flows).
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: 1 of 20 files reviewed, 2 unresolved discussions, 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 (waiting on @kailun-qin)
tools/sgx/ra-tls/ra_tls_common.h
line 6 at r3 (raw file):
Or we actually don't consider ra_tls_attest as external RA-TLS implementation?
Yes, we don't consider "attest" part as external thing. In other words, RA-TLS plugins must not change the attesting part, only the verification part.
This is because attesting is simple and never changes -- the Gramine enclave simply generates the SGX quote and sends it as an attestation evidence. This part never changes (at least for now).
If there will ever be a need for modifying the attesting part, we will create follow-up PRs. For now, I haven't encountered real use cases that would require anything other than changes in the verification part.
tools/sgx/ra-tls/secret_prov_common.h
line 5 at r3 (raw file):
Do we expect users to have their own implementations of Secret Prov (even providing different user APIs)?
No, we don't expect users to do that. The Secret Prov library must always have the same APIs (it's just the internal verification flows that change in plugins).
Then it's a bit confusing here - in the current version of PR, it's
secret_prov_verify
(incl. bothsecret_prov_common
andsecret_prov_verify
) we actually supplied and we should expect this to be used for external implementation (e.g., just different in the underlying attestation flows).
To be honest, I can't understand what you're saying here.
But IIUC, your point is that "plugin" SecretProv doesn't really need to implement anything? It just needs to link against the correct RA-TLS libraries? This is true (at least in my current MAA implementation, and I don't really expect other plugin implementations to be different).
So maybe your question is: why do we expose secret_prov_common.h
to end users at all? I guess you're right, and this particular header file can be just an internal implementation detail, and then this header file should not be put in the installation header path.
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: 1 of 20 files reviewed, 2 unresolved discussions, 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 (waiting on @dimakuv)
tools/sgx/ra-tls/ra_tls_common.h
line 6 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Or we actually don't consider ra_tls_attest as external RA-TLS implementation?
Yes, we don't consider "attest" part as external thing. In other words, RA-TLS plugins must not change the attesting part, only the verification part.
This is because attesting is simple and never changes -- the Gramine enclave simply generates the SGX quote and sends it as an attestation evidence. This part never changes (at least for now).
If there will ever be a need for modifying the attesting part, we will create follow-up PRs. For now, I haven't encountered real use cases that would require anything other than changes in the verification part.
Good to know, thanks for the explanation!
tools/sgx/ra-tls/secret_prov_common.h
line 5 at r3 (raw file):
To be honest, I can't understand what you're saying here.
Yep, I don't understand what I was saying neither - just found it confusing at that time ;).
But IIUC, your point is that "plugin" SecretProv doesn't really need to implement anything? It just needs to link against the correct RA-TLS libraries?
Yes, this is indeed my point.
So maybe your question is: why do we expose secret_prov_common.h to end users at all?
And then yes, this is exactly my question (thanks!). I'd expect to not have it exposed at all.
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: 1 of 20 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
tools/sgx/ra-tls/secret_prov_common.h
line 5 at r3 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
To be honest, I can't understand what you're saying here.
Yep, I don't understand what I was saying neither - just found it confusing at that time ;).
But IIUC, your point is that "plugin" SecretProv doesn't really need to implement anything? It just needs to link against the correct RA-TLS libraries?
Yes, this is indeed my point.
So maybe your question is: why do we expose secret_prov_common.h to end users at all?
And then yes, this is exactly my question (thanks!). I'd expect to not have it exposed at all.
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 14 of 19 files at r1, 2 of 3 files at r2, 1 of 3 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
This commit splits RA-TLS and SecretProv libs in a more fine-grained manner, suitable for writing external plugins (e.g. with Microsoft Azure Attestation flows). This commit also installs corresponding header files and pkg-config files. Finally, this commit fixes a bug with too-many exported symbols from these libs. The examples `ra-tls-mbedtls` and `ra-tls-secret-prov` are updated to reflect these changes. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
b52b9dc
to
64a87bb
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 r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Description of the changes
This PR splits RA-TLS and SecretProv libs in a more fine-grained manner, suitable for writing external plugins (e.g. with Microsoft Azure Attestation flows). This PR also installs corresponding header files and pkg-config files. Finally, this PR fixes a bug with too-many exported symbols from these libs.
For more context on this, see #1071.
Fixes #1182 (by making the
abort
symbol local)How to test this PR?
The examples
ra-tls-mbedtls
andra-tls-secret-prov
(run in CI) are updated to reflect these changes.Notes for reviewers
To aid the review process, let me show some logs how things look with this PR.
This change is