-
Notifications
You must be signed in to change notification settings - Fork 19
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 SGX enclave signing with OpenSSL engine #19
base: master
Are you sure you want to change the base?
Add SGX enclave signing with OpenSSL engine #19
Conversation
Signed-off-by: Sankaranarayanan Venkatasubramanian <sankaranarayanan.venkatasubramanian@intel.com>
Signed-off-by: Sankaranarayanan Venkatasubramanian <sankaranarayanan.venkatasubramanian@intel.com>
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 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 12 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 @svenkata9)
Integrations/openssl/engine-sign/gramine-sgx-ossl-sign
line 20 at r2 (raw file):
def sign_with_ossl_engine(data, engine, key): """Signs *data* using *key* from OpenSSL *engine*
Please add a dot at the end.
Integrations/openssl/engine-sign/gramine-sgx-ossl-sign
line 26 at r2 (raw file):
Suitable to be used as a callback to :py:func:`graminelibos.Sigstruct.sign()`. This function requires that a valid OpenSSL engine is installed/enabled, the user has access to the engine and a 3072-bit RSA key created in the engine that is enabled for signing.
Please remove trailing whitespace.
Integrations/openssl/engine-sign/gramine-sgx-ossl-sign
line 54 at r2 (raw file):
signature_int = int.from_bytes(signature, byteorder='big') return exponent_int, modulus_int, signature_int
The code looks reasonable, though I have no experience with OpenSSL Engine API. But from quick googling, looks legit.
As far as this was tested (and it was, by Intel), I'm fine with this implementation.
Integrations/openssl/engine-sign/README.md
line 1 at r2 (raw file):
# Enclave signing with a HSM using OpenSSL engine
an HSM
Integrations/openssl/engine-sign/README.md
line 7 at r2 (raw file):
deployments, you should use a key secured in a Hardware Security Module (HSM). This directory contains the plugin to Gramine tools and templates that enable
and templates
? I think you should just delete these two words.
Integrations/openssl/engine-sign/README.md
line 7 at r2 (raw file):
deployments, you should use a key secured in a Hardware Security Module (HSM). This directory contains the plugin to Gramine tools and templates that enable
enable
-> enables
Integrations/openssl/engine-sign/README.md
line 8 at r2 (raw file):
This directory contains the plugin to Gramine tools and templates that enable support for production signing of SGX enclaves using keys from a HSM accessible
an HSM
Integrations/openssl/engine-sign/README.md
line 9 at r2 (raw file):
This directory contains the plugin to Gramine tools and templates that enable support for production signing of SGX enclaves using keys from a HSM accessible using OpenSSL engine.
Could you give a link to OpenSSL engine description? This one looks the most official: https://www.openssl.org/docs/man1.1.1/man1/openssl-engine.html
Integrations/openssl/engine-sign/README.md
line 13 at r2 (raw file):
## Prerequisites for SGX enclave signing - OpenSSL engine-based access available for the HSM.
I can't parse this sentence. What does it mean exactly?
Integrations/openssl/engine-sign/README.md
line 15 at r2 (raw file):
- OpenSSL engine-based access available for the HSM. - 3072-bit RSA private key with a public exponent 3 created in this HSM. Please note that only Managed HSM supports setting public exponent.
Please note that only Managed HSM ...
-- remnant from Azure KeyVault? This sentence should be removed.
Integrations/openssl/engine-sign/README.md
line 16 at r2 (raw file):
- 3072-bit RSA private key with a public exponent 3 created in this HSM. Please note that only Managed HSM supports setting public exponent. - Tools installed and the user is authenticated to access to the HSM.
Which tools? Is there something more specific that could be said? Or maybe a link to some tutorial which explains how a hardware HSM can be used with OpenSSL Engine?
Integrations/openssl/engine-sign/README.md
line 20 at r2 (raw file):
## Running The command to sign the enclave with a HSM using OpenSSL engine looks like this:
with an HSM
Integrations/openssl/engine-sign/README.md
line 29 at r2 (raw file):
where `sgx_sign_key` is the name of the RSA private key created in the HSM accessible through the engine `openssl_engine`.
Please remove this empty line.
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, 12 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)
Integrations/openssl/engine-sign/README.md
line 7 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
and templates
? I think you should just delete these two words.
I should have mentioned it. There is this PR in GSC (gramineproject/gsc#112) that enables --template
option for signing (with HSMs or user preferred way). I wanted to avoid two PRs (once now and to update readme later), hence added the wording. I have the templates ready and once the template PR gets merged into GSC repo and the ossl engine sign gets merged to contrib, I will just add the templates in a new PR without having to update the readme.
Integrations/openssl/engine-sign/README.md
line 7 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
enable
->enables
with templates, and so enables
is valid. (please see comment above)
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, 11 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 @svenkata9)
Integrations/openssl/engine-sign/README.md
line 7 at r2 (raw file):
Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…
I should have mentioned it. There is this PR in GSC (gramineproject/gsc#112) that enables
--template
option for signing (with HSMs or user preferred way). I wanted to avoid two PRs (once now and to update readme later), hence added the wording. I have the templates ready and once the template PR gets merged into GSC repo and the ossl engine sign gets merged to contrib, I will just add the templates in a new PR without having to update the readme.
Ok, but please also take my comments in #20 into account (I requested to modify the wording slightly).
Signed-off-by: Sankaranarayanan Venkatasubramanian sankaranarayanan.venkatasubramanian@intel.com
This adds a plugin
gramine-sgx-ossl-sign
to core Gramine. This pluginallows to sign SGX enclaves with private keys stored in a HSM using OpenSSL engine.
See also gramineproject/gsc#139 for additional discussions.
This change is