Introduce sgx.remote_attestation = "[none|epid|dcap]"
#638
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 29 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)
a discussion (no related file):
TODO(myself): Currently tested on my local machine with EPID. Must also test with DCAP on MS Azure VM.
a discussion (no related file):
@NieDzejkob Could I ask you to review this PR? You were the last one who was working on SGX attestation and examples.
CI-Examples/python/README.md line 25 at r1 (raw file):
If you want to run the `scripts/sgx-quote.py` script, you must build the example with SGX remote attestation enabled. By default, the example is built *without* remote attestation.
FYI: This became annoying quickly that to run a HelloWorld script in Python, I had to set up SGX remote attestation. Now by default it is sgx.remote_attestation = "none", and Python scripts do not fail now, if SGX attestation envvars were not set.
CI-Examples/ra-tls-mbedtls/Makefile line 9 at r1 (raw file):
# for DCAP/ECDSA attestation, specify SPID as empty string (linkable value is ignored) RA_CLIENT_SPID ?= RA_CLIENT_LINKABLE ?= 0
FYI: Here and everywhere, this was not needed. We can embed envvars directly in the manifest.template file, no need for this level of indirection.
CI-Examples/ra-tls-mbedtls/README.md line 94 at r1 (raw file):
make app epid gramine-sgx ./server epid &
I can technically remove this argv[1] == epid|dcap from the applications (server and client). Do we want to have an explicit command-line argument, or should we rewrite the app code so that apps find which attestation scheme to use themselves?
For now, I decided to keep it explicit, but I can change.
CI-Examples/ra-tls-mbedtls/src/server.c line 143 at r1 (raw file):
if (strcmp(argv[1], attestation_type_str)) { mbedtls_printf("User requested RA-TLS attestation of type '%s' but Gramine has RA-TLS " "attestation of type '%s'\n", argv[1], attestation_type_str);
@mkow Specially for Michal :)
Documentation/tutorials/pytorch/index.rst line 498 at r1 (raw file):
<https://github.com/gramineproject/gramine/tree/master/Pal/src/host/Linux-SGX/tools#secret-provisioning-libraries>`__:: sgx.remote_attestation = "dcap"
FYI: Please note that this PyTorch tutorial works only with DCAP attestation, so I hard-code it here.
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
assert(g_attestation_type_str); char* str = alloc_concat(g_attestation_type_str, /*a_len=*/-1, "\n", /*b_len=*/-1);
FYI: Here I'm having e.g. epid\n in the /dev/attestation/attestation_type file. The newline is purely for convinience, e.g., when a user cats this file in Busybox. I can remove the newline, if people consider it more correct (but from what I understand, Linux typically adds the newline to its pseudo-files).
LibOS/shim/test/regression/attestation.manifest.template line 21 at r1 (raw file):
fs.mount.bin.type = "chroot" fs.mount.bin.path = "/bin" fs.mount.bin.uri = "file:/bin"
FYI: As you can notice, I removed some deprecated options from this file and rewrote to newer syntax. This is fine because I introduce attestation_deprecated_syntax test, which contains all these deprecated options -- so we still test them.
Pal/src/host/Linux-SGX/sgx_main.c line 758 at r1 (raw file):
"Detected EPID remote attestation parameters 'ra_client_spid' and/or " "'ra_client_linkable' in the manifest but no 'remote_attestation' parameter. " "Please add 'sgx.remote_attestation = true' to the manifest.");
FYI: I simply removed this check because I don't know how it helps anything.
Pal/src/host/Linux-SGX/sgx_platform.c line 78 at r1 (raw file):
err: DO_SYSCALL(close, sock); log_error("Cannot connect to AESM service (tried " AESM_SOCKET_NAME_LEGACY " and "
FYI: This is slight renaming, just because I noticed this and decided to use a proper googlable term here.
There was a problem hiding this comment.
Reviewable status: 0 of 29 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)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself): Currently tested on my local machine with EPID. Must also test with DCAP on MS Azure VM.
Done. See the updated PR description.
There was a problem hiding this comment.
Reviewable status: 0 of 29 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)
a discussion (no related file):
TODO(myself): I forgot to update _DkAttestationQuote() which failed on a DCAP-attestation Azure VM, on a LibOS regression test attestation.
There was a problem hiding this comment.
Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 26 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 @dimakuv)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
env.CARGO_HOME = env.WORKSPACE + '/CARGO_HOME' env.RA_TYPE = 'epid'
Could we move this to makefiles generating manifests in each example? This might get annoying if you set this variable and forget about it.
CI-Examples/busybox/README.md line 7 at r1 (raw file):
## Building with SGX remote attestation
Not blocking, but maybe we should link to some of our docs about RA here? Not sure if everyone reading this will be familiar with RA.
CI-Examples/busybox/README.md line 15 at r1 (raw file):
a working DCAP setup. Then build the example as follows:RA_TYPE=dcap make SGX=1
This looks weird, when you put some variables before make and some after
CI-Examples/busybox/README.md line 22 at r1 (raw file):
linkable quotes or not:RA_TYPE=epid RA_CLIENT_SPID=12345678901234567890123456789012 RA_CLIENT_LINKABLE=0 make SGX=1
ditto
CI-Examples/busybox/README.md line 27 at r1 (raw file):
The above dummy values will suffice for simple experiments, but if you wish to run `sgx-quote.py` and verify the output, you will need to provide an [SPID recognized by Intel][spid].
Link missing
CI-Examples/python/README.md line 25 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: This became annoying quickly that to run a HelloWorld script in Python, I had to set up SGX remote attestation. Now by default it is
sgx.remote_attestation = "none", and Python scripts do not fail now, if SGX attestation envvars were not set.
Nice, I've hit this issue multiple times, each time wondering what did I do wrong (and ofc it was missing SPID and such).
CI-Examples/python/README.md line 30 at r1 (raw file):
a working DCAP setup. Then build the example as follows:RA_TYPE=dcap make SGX=1
ditto (group variables)
CI-Examples/python/README.md line 38 at r1 (raw file):
RA_TYPE=epid RA_CLIENT_SPID=12345678901234567890123456789012 RA_CLIENT_LINKABLE=0 make SGX=1
ditto
CI-Examples/ra-tls-mbedtls/README.md line 94 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I can technically remove this
argv[1] == epid|dcapfrom the applications (server and client). Do we want to have an explicit command-line argument, or should we rewrite the app code so that apps find which attestation scheme to use themselves?For now, I decided to keep it explicit, but I can change.
The argument passed here must match the one in the manifest, right? I would remove it then, it's just confusing (that you have an argument, that you cannot really change).
CI-Examples/ra-tls-mbedtls/src/server.c line 71 at r1 (raw file):
FILE* f = fopen(path, "r"); if (!f) return -1;
Could we have the usual return code convention?
CI-Examples/ra-tls-mbedtls/src/server.c line 135 at r1 (raw file):
return 1; } attestation_type_str[ret - 1] = '\0';
You could just pass sizeof() - 1 to file_read instead. This version handles ret == 0 incorrectly.
CI-Examples/ra-tls-mbedtls/src/server.c line 139 at r1 (raw file):
char* newline_pos = strchr(attestation_type_str, '\n'); if (newline_pos) *newline_pos = '\0';
just 0 please
CI-Examples/ra-tls-mbedtls/src/server.c line 141 at r1 (raw file):
*newline_pos = '\0'; if (strcmp(argv[1], attestation_type_str)) {
Wouldn't it be easier to just throw argv away?
Documentation/attestation.rst line 230 at r1 (raw file):
- ``/dev/attestation/attestation_type`` file contains the name of the attestation scheme used, currently one of ``none``, ``epid``, ``dcap`` and ``unclear`` (the latter is used when attestation type cannot be identified).
Maybe unknown? unclear sounds weird to me
Documentation/attestation.rst line 232 at r1 (raw file):
``unclear`` (the latter is used when attestation type cannot be identified). The contents of the file end with a newline, so applications are advised to strip the newline symbol (``\n``) from the read characters.
If we expect everybody to strip the newline, then why we provide it?
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: Here I'm having e.g.
epid\nin the/dev/attestation/attestation_typefile. The newline is purely for convinience, e.g., when a user cats this file in Busybox. I can remove the newline, if people consider it more correct (but from what I understand, Linux typically adds the newline to its pseudo-files).
I would remove it. I would expect most of the usages of this file to be programmed and they would need to remove this newline. In sh you can do cat file ; echo if you want a newline.
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
ret = toml_string_in(g_manifest_root, "sgx.remote_attestation", &g_attestation_type_str); if (ret < 0) { /* TODO: Bool syntax is deprecated in v1.3, remove 2 versions later. */
Btw, why this 2 versions delay? IIUC this is so people have time to fix their manifests - one version should be enough (i.e. v1.3 would print a warning and v1.4 would hard fail). Were there some arguments against such approach?
LibOS/shim/src/fs/dev/attestation.c line 407 at r1 (raw file):
if (sgx_remote_attestation_enabled) g_attestation_type_str = strdup("unclear"); /* don't bother, it's deprecated anyway */ else
{ } please (since it's if-else)
LibOS/shim/src/fs/dev/attestation.c line 410 at r1 (raw file):
g_attestation_type_str = strdup("none"); }
We should check the string for allowed values here (user might write sgx.remote_attestation = "epdi" or something)
LibOS/shim/src/fs/dev/attestation.c line 415 at r1 (raw file):
/* always add /dev/attestation/attestation_type file, even if it is "none" */ pseudo_add_str(attestation, "attestation_type", &attestation_type_load);
& not needed
LibOS/shim/src/fs/dev/attestation.c line 448 at r1 (raw file):
int init_attestation(struct pseudo_node* dev) { int ret;
Could you just move this to the first usage? There is only one anyway.
LibOS/shim/test/regression/test_libos.py line 493 at r1 (raw file):
def test_002_attestation(self): stdout, _ = self.run_binary(['attestation_deprecated_syntax'], timeout=60)
Why the custom timeout?
LibOS/shim/test/regression/test_libos.py line 499 at r1 (raw file):
def test_003_attestation_stdio(self): stdout, _ = self.run_binary(['attestation_deprecated_syntax', 'test_stdio'], timeout=60)
ditto
Pal/src/host/Linux-SGX/sgx_main.c line 754 at r1 (raw file):
bool sgx_remote_attestation_enabled; ret = toml_bool_in(manifest_root, "sgx.remote_attestation", /*defaultval=*/false, &sgx_remote_attestation_enabled);
Wrong indent
Pal/src/host/Linux-SGX/sgx_platform.c line 156 at r1 (raw file):
if (r->errorcode != 0) { log_error("AESM service returned error %d; this may indicate that Gramine requested " "an unsupported SGX remote attestation type (EPID), please verify "
Hm, what (EPID) means here? This case is for epid, so how can it also be unsupported?
Pal/src/host/Linux-SGX/sgx_platform.c line 195 at r1 (raw file):
if (r->errorcode != 0) { log_error("AESM service returned error %d; this may indicate that Gramine requested " "an unsupported SGX remote attestation type (DCAP/ECDSA), please verify "
ditto
There was a problem hiding this comment.
Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 33 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 @boryspoplawski and @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@NieDzejkob Could I ask you to review this PR? You were the last one who was working on SGX attestation and examples.
I don't think this makes me anywhere near an expert on this, but sure, I'll take a look.
CI-Examples/busybox/README.md line 15 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This looks weird, when you put some variables before
makeand some after
These variables have different semantics, though. The ones specified before make will be available in the environment of all child processes, while SGX=1 is local to the Makefile. I believe the current version is correct.
CI-Examples/python/scripts/sgx-quote.py line 11 at r1 (raw file):
if not os.path.exists("/dev/attestation/user_report_data"): print("Cannot find `/dev/attestation/user_report_data`; " "are you running under SGX and did you enable remote attestation?")
This reads weird. How about are you running under SGX, with remote attestation enabled?
CI-Examples/ra-tls-mbedtls/client.manifest.template line 27 at r1 (raw file):
sgx.thread_num = 4 sgx.remote_attestation = "{{ env.get('RA_TYPE', 'none') }}"
Hold on. What is happening here? Will the client enclave actually be attesting itself? If so, it looks like something's missing as far as EPID attestation is concerned. Otherwise I don't see why remote_attestation = true was set before in the first place.
Might be worth a comment or two to clarify.
Documentation/manifest-syntax.rst line 739 at r1 (raw file):
your registered Intel SGX EPID Attestation Service credentials (linkable/unlinkable mode and SPID of the client respectively).
Perhaps we should link to the sgx-intro glossary, where we have some instructions for obtaining these?
Code quote:
For EPID based attestation, ``remote_attestation`` must be set to ``epid``.
In addition, ``ra_client_linkable`` and ``ra_client_spid`` must be filled with
your registered Intel SGX EPID Attestation Service credentials
(linkable/unlinkable mode and SPID of the client respectively).LibOS/shim/src/fs/dev/attestation.c line 406 at r1 (raw file):
} if (sgx_remote_attestation_enabled) g_attestation_type_str = strdup("unclear"); /* don't bother, it's deprecated anyway */
I think it'd be better to just distinguish EPID vs DCAP like old Gramine did. That way we'd avoid a weird special case the user's code might need to handle.
Pal/src/host/Linux-SGX/sgx_internal.h line 44 at r1 (raw file):
SGX_ATTESTATION_EPID, SGX_ATTESTATION_DCAP, SGX_ATTESTATION_UNCLEAR, /* to support legacy `sgx.remote_attestation = true` (EPID or DCAP) */
Same here — wouldn't it be better to distinguish at parse time?
Pal/src/host/Linux-SGX/sgx_main.c line 758 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: I simply removed this check because I don't know how it helps anything.
It might help the user arrive at a working manifest. Always better than silently refusing to work.
python/graminelibos/sgx_sign.py line 496 at r1 (raw file):
linkable = manifest_sgx.get('ra_client_linkable', False) print('SGX remote attestation:') print(f' {attestation_type}, spid = `{spid}`, linkable = {linkable}')
I think the previous output was less confusing. This one kinda signals that spid and linkable matter even for dcap attestation, which is not the case.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 33 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 @dimakuv)
CI-Examples/busybox/README.md line 15 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
These variables have different semantics, though. The ones specified before
makewill be available in the environment of all child processes, whileSGX=1is local to theMakefile. I believe the current version is correct.
All of them (i.e. both env variables and arguments to make) will be visible as env variables in all child processes.
The only difference is that arguments are not overrideable by default.
There was a problem hiding this comment.
Reviewable status: 24 of 35 files reviewed, 32 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 @boryspoplawski, @dimakuv, @mkow, and @NieDzejkob)
-- commits line 31 at r2:
FYI: I detected this discrepancy in parsing when testing on a MS Azure CC VM, which showed me that there is a bug in my manifest parsing. This got me annoyed because I had parsing of sgx.remote_attestation and co. in three different places: in untrusted PAL, in trusted PAL and in LibOS. This design was error prone.
So I added this fixup commit -- even before looking at the reviews left by Borys and Jakub -- to have a single place with manifest parsing for remote attestation. No review comments were taken into account in this fixup commit (other than a couple from Borys that got incorporated accidentally).
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Btw, why this 2 versions delay? IIUC this is so people have time to fix their manifests - one version should be enough (i.e. v1.3 would print a warning and v1.4 would hard fail). Were there some arguments against such approach?
@woju came up with this policy. I don't know the exact reason about "2 versions delay".
LibOS/shim/src/fs/dev/attestation.c line 406 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I think it'd be better to just distinguish EPID vs DCAP like old Gramine did. That way we'd avoid a weird special case the user's code might need to handle.
OK, this is irrelevant now. The unclear status can never show up at the LibOS level, only none, dcap or epid.
LibOS/shim/src/fs/dev/attestation.c line 407 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
{ }please (since it's if-else)
Done. Not relevant anymore.
LibOS/shim/src/fs/dev/attestation.c line 410 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
We should check the string for allowed values here (user might write
sgx.remote_attestation = "epdi"or something)
Done. Not relevant anymore as LibOS now takes it from the PAL's global public state -- which sanitizes the values and allows only none, epid, dcap.
LibOS/shim/test/regression/test_libos.py line 493 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why the custom timeout?
attestation test may be slow because it tests for resource leakage. So the default 20 seconds may not be enough.
LibOS/shim/test/regression/test_libos.py line 499 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto
ditto
Pal/src/host/Linux-SGX/meson.build line 55 at r2 (raw file):
libpal_sgx = shared_library('pal', 'common_parser.c',
FYI: I wasn't sure where to put the file that is common only to untrusted PAL and trusted PAL. So I just added it in both lists explicitly. Any better ideas?
Pal/src/host/Linux-SGX/sgx_internal.h line 44 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Same here — wouldn't it be better to distinguish at parse time?
Well, yes, at parse time we distinguish between these and the final attestation_type can never be UNCLEAR. So this UNCLEAR value is only an intermediate one, for parsing purposes.
Is this a bad smell? Should I remove this UNCLEAR value from this enum, and just use a helper variable in the parsing func?
Pal/src/host/Linux-SGX/sgx_main.c line 754 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Wrong indent
Done, not relevant anymore.
Pal/src/host/Linux-SGX/sgx_main.c line 758 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
It might help the user arrive at a working manifest. Always better than silently refusing to work.
I meant that in the new syntax, it is kinda useless. This check was supposed to fail on manifests like this:
sgx.remote_attestation == false
sgx.ra_client_spid = "12345678abcd"
In the new syntax, we instead have it more explicit:
sgx.remote_attestation == "none"
sgx.ra_client_spid = "12345678abcd"
I think it is reasonable to not fail in such a manifest. It can be read as: the user added the SPID for testing purposes, but then switched to none attestation. So the user explicitly forbid the attestation to happen, and kept the SPID for the case when the user wants to quickly switch and test epid.
Or am I overthinking it? Maybe it's indeed better to fail? @mkow @boryspoplawski
Pal/src/host/Linux-SGX/sgx_platform.c line 156 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Hm, what
(EPID)means here? This case is for epid, so how can it also be unsupported?
Done. Rephrased.
Pal/src/host/Linux-SGX/sgx_platform.c line 195 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto
Done. Rephrased.
There was a problem hiding this comment.
Reviewable status: 24 of 35 files reviewed, 31 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 @boryspoplawski, @dimakuv, @mkow, and @NieDzejkob)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself): I forgot to update
_DkAttestationQuote()which failed on a DCAP-attestation Azure VM, on a LibOS regression testattestation.
Done. The fix is in the first fixup commit.
There was a problem hiding this comment.
Reviewable status: 24 of 35 files reviewed, 30 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
CI-Examples/ra-tls-mbedtls/README.md line 94 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
The argument passed here must match the one in the manifest, right? I would remove it then, it's just confusing (that you have an argument, that you cannot really change).
+1 to removing it.
CI-Examples/ra-tls-mbedtls/src/server.c line 143 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@mkow Specially for Michal :)
yay!
Documentation/manifest-syntax.rst line 739 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Perhaps we should link to the sgx-intro glossary, where we have some instructions for obtaining these?
The glossary is a bit Gramine-independent, I think this stuff should rather stay here (your quote is purely "how to configure Gramine to do XYZ" and not an explanation of some SGX concept).
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@woju came up with this policy. I don't know the exact reason about "2 versions delay".
I'd be fine with a shorter grace period.
Pal/src/host/Linux-SGX/sgx_main.c line 758 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I meant that in the new syntax, it is kinda useless. This check was supposed to fail on manifests like this:
sgx.remote_attestation == false sgx.ra_client_spid = "12345678abcd"In the new syntax, we instead have it more explicit:
sgx.remote_attestation == "none" sgx.ra_client_spid = "12345678abcd"I think it is reasonable to not fail in such a manifest. It can be read as: the user added the SPID for testing purposes, but then switched to
noneattestation. So the user explicitly forbid the attestation to happen, and kept the SPID for the case when the user wants to quickly switch and testepid.Or am I overthinking it? Maybe it's indeed better to fail? @mkow @boryspoplawski
I think the current behavior is fine (to allow this) for debugging purposes. But if it turns out that it confuses users then we should change it.
There was a problem hiding this comment.
Reviewed 6 of 11 files at r2, all commit messages.
Reviewable status: 30 of 35 files reviewed, 34 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
CI-Examples/busybox/README.md line 15 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
All of them (i.e. both env variables and arguments to make) will be visible as env variables in all child processes.
The only difference is that arguments are not overrideable by default.
Okay, my bad.
Still, we have some other axes along which consistency would matter — commands other than make also need to be provided with the RA env vars sometimes, so it would be good to always have those before the command. While SGX=1 is put after make in all the examples, so it would also be weird to do SGX=1 make in this case.
Documentation/manifest-syntax.rst line 739 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
The glossary is a bit Gramine-independent, I think this stuff should rather stay here (your quote is purely "how to configure Gramine to do XYZ" and not an explanation of some SGX concept).
Yes, but when we say "put your SPID here", it'd be useful to link to "what an SPID is and how do I get one".
Pal/src/host/Linux-SGX/common_parser.c line 45 at r2 (raw file):
&sgx_remote_attestation_enabled); if (ret < 0) { log_error("Cannot parse 'sgx.remote_attestation' (the value must be \"none\", \"epid\" "
Hmm. So a bad type will list the possible options, but an unexpected string will just say "Unknown 'sgx.remote_attestation' type". I think it would make more sense if the same error message was emitted.
Pal/src/host/Linux-SGX/common_parser.c line 61 at r2 (raw file):
ret = -EINVAL; goto out; }
AFAICS we could parse this first, before the attestation type itself. Then you could put the logic to distinguish the legacy syntax where the attestation_type = SGX_ATTESTATION_UNCLEAR line currently is, and refactor out this enum variant entirely.
I think we also have somewhat inconsistent behavior here: if the attestation type is not epid, then a wrong type for ra_client_linkable will be ignored, but ra_client_spid will not. I don't think supporting sgx.ra_client_spid = [] or something makes sense anyway, but we should be consistent in detecting that for each parameter.
Code quote:
ret = toml_string_in(manifest_root, "sgx.ra_client_spid", &sgx_ra_client_spid_str);
if (ret < 0) {
log_error("Cannot parse 'sgx.ra_client_spid'");
ret = -EINVAL;
goto out;
}Pal/src/host/Linux-SGX/common_parser.c line 93 at r2 (raw file):
ret = -PAL_ERROR_INVAL; goto out; }
Why is this done by both parse_attestation_epid_params and parse_attestation_type?
Code quote:
ret = toml_string_in(manifest_root, "sgx.ra_client_spid", &sgx_ra_client_spid_str);
if (ret < 0) {
log_error("Cannot parse 'sgx.ra_client_spid'");
ret = -PAL_ERROR_INVAL;
goto out;
}Pal/src/host/Linux-SGX/common_parser.c line 111 at r2 (raw file):
goto out; } spid[i / 2] = spid[i / 2] * 16 + (uint8_t)val;
We already have parse_hex in Pal/src/host/Linux-SGX/tools/common/util.c, perhaps it would be a good idea to move that to common/ instead of duplicating an ad-hoc implementation here?
Code quote:
/* sgx.ra_client_spid must be hex string */
for (size_t i = 0; i < strlen(sgx_ra_client_spid_str); i++) {
int8_t val = hex2dec(sgx_ra_client_spid_str[i]);
if (val < 0) {
log_error("Malformed 'sgx.ra_client_spid' value in the manifest: %s",
sgx_ra_client_spid_str);
ret = -PAL_ERROR_INVAL;
goto out;
}
spid[i / 2] = spid[i / 2] * 16 + (uint8_t)val;
}Pal/src/host/Linux-SGX/db_main.c line 635 at r2 (raw file):
case SGX_ATTESTATION_EPID: g_pal_public_state.attestation_type = "epid"; break; case SGX_ATTESTATION_DCAP: g_pal_public_state.attestation_type = "dcap"; break; default: BUG();
Do we really want to make a stringly-typed variable a part of our public interface? This reeks of JavaScript. Why not store the enum value itself in g_pal_public_state?
Code quote:
switch (attestation_type) {
case SGX_ATTESTATION_NONE: g_pal_public_state.attestation_type = "none"; break;
case SGX_ATTESTATION_EPID: g_pal_public_state.attestation_type = "epid"; break;
case SGX_ATTESTATION_DCAP: g_pal_public_state.attestation_type = "dcap"; break;
default: BUG();
}Pal/src/host/Linux-SGX/sgx_main.c line 758 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think the current behavior is fine (to allow this) for debugging purposes. But if it turns out that it confuses users then we should change it.
What about the situation when sgx.remote_attestation is not set at all?
There was a problem hiding this comment.
Reviewed 11 of 11 files at r2.
Reviewable status: 24 of 35 files reviewed, 36 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
LibOS/shim/src/fs/dev/attestation.c line 393 at r2 (raw file):
return 0; g_attestation_type_str = strdup(g_pal_public_state->attestation_type);
Can't we just use g_pal_public_state->attestation_type directly? Why the copying?
Pal/include/pal/pal.h line 87 at r2 (raw file):
struct pal_public_state { const char* host_type; const char* attestation_type; /* currently only for Linux-SGX */
Is it a good idea to put PAL specific info here?
Pal/src/host/Linux-SGX/common_parser.c line 1 at r2 (raw file):
/* SPDX-License-Identifier: LGPL-3.0-or-later */
This file needs a better name (to include it parses sgx specific manifest options).
Pal/src/host/Linux-SGX/common_parser.c line 35 at r2 (raw file):
} else { log_error("Unknown 'sgx.remote_attestation' type"); ret = -EINVAL;
This function should return PAL error codes
Pal/src/host/Linux-SGX/common_parser.c line 43 at r2 (raw file):
bool sgx_remote_attestation_enabled; ret = toml_bool_in(manifest_root, "sgx.remote_attestation", /*defaultval=*/false, &sgx_remote_attestation_enabled);
wrong alignment
Pal/src/host/Linux-SGX/common_parser.c line 47 at r2 (raw file):
log_error("Cannot parse 'sgx.remote_attestation' (the value must be \"none\", \"epid\" " "or \"dcap\", or in case of legacy syntax `true` or `false`)"); ret = -EINVAL;
ditto (PAL error codes)
Pal/src/host/Linux-SGX/common_parser.c line 59 at r2 (raw file):
if (ret < 0) { log_error("Cannot parse 'sgx.ra_client_spid'"); ret = -EINVAL;
ditto (PAL error codes)
Pal/src/host/Linux-SGX/common_parser.c line 64 at r2 (raw file):
/* legacy syntax: EPID is used if SPID is a non-empty string in manifest, otherwise DCAP */ if (attestation_type == SGX_ATTESTATION_UNCLEAR) {
This seem to be the only usage of SGX_ATTESTATION_UNCLEAR now, so we should get rid of it.
Pal/src/host/Linux-SGX/sgx_internal.h line 44 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Well, yes, at parse time we distinguish between these and the final
attestation_typecan never beUNCLEAR. So thisUNCLEARvalue is only an intermediate one, for parsing purposes.Is this a bad smell? Should I remove this
UNCLEARvalue from this enum, and just use a helper variable in the parsing func?
Yes please
Pal/src/host/Linux-SGX/sgx_main.c line 758 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
What about the situation when
sgx.remote_attestationis not set at all?
When debugging I would actually comment this line, not change it to "none", so it's fine for me. The only issue would be that if somebody forgot to put this line, but they would notice they are not doing any attestation :) Hopefully...
Pal/src/host/Linux-SGX/sgx_main.c line 734 at r2 (raw file):
ret = parse_attestation_type(manifest_root, &enclave_info->attestation_type); if (ret < 0) { /* error is already printed by the called func */
Will need to translate PAL -> UNIX errno here.
There was a problem hiding this comment.
Reviewable status: 24 of 35 files reviewed, 36 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Could we move this to makefiles generating manifests in each example? This might get annoying if you set this variable and forget about it.
What do you want exactly? Do you want to move RA_TYPE to other Jenkinsfiles (close to invocations make SGX=1 ...) under .ci/, to make the commands with which we build and run CI tests more explicit?
CI-Examples/busybox/README.md line 7 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Not blocking, but maybe we should link to some of our docs about RA here? Not sure if everyone reading this will be familiar with RA.
Done
CI-Examples/busybox/README.md line 15 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
All of them (i.e. both env variables and arguments to make) will be visible as env variables in all child processes.
The only difference is that arguments are not overrideable by default.
Done. Moved to before make. We now have a discrepancy with other READMEs, but I wouldn't care much.
CI-Examples/busybox/README.md line 22 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto
Done.
CI-Examples/busybox/README.md line 27 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Link missing
Done.
CI-Examples/python/README.md line 30 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto (group variables)
Done.
CI-Examples/python/README.md line 38 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto
Done.
CI-Examples/python/scripts/sgx-quote.py line 11 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
This reads weird. How about
are you running under SGX, with remote attestation enabled?
Done.
CI-Examples/ra-tls-mbedtls/client.manifest.template line 27 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Hold on. What is happening here? Will the client enclave actually be attesting itself? If so, it looks like something's missing as far as EPID attestation is concerned. Otherwise I don't see why
remote_attestation = truewas set before in the first place.Might be worth a comment or two to clarify.
Done. You are right, this was not needed at all. Removed it. Apparently we thought that to run SGX quote verification on the client, we need some support from the AESM daemon in Gramine, but this is not so.
CI-Examples/ra-tls-mbedtls/client.manifest.template line 8 at r3 (raw file):
loader.log_level = "{{ log_level }}" loader.env.LD_LIBRARY_PATH = "/lib:/usr/local/lib:{{ arch_libdir }}:/usr{{ arch_libdir }}"
FYI: This /usr/local/lib is needed because the in-SGX-enclave DCAP version of the client (client_dcap) is looking for libdcap_quoteverify.so lib which is installed under this directory, at least on Ubuntu.
CI-Examples/ra-tls-mbedtls/README.md line 94 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
+1 to removing it.
Done.
Note that this is only on the server side. The client still needs to be provisioned with an explicit command-line argument native|epid|dcap, because the client may run outside of Gramine and has no "hint" in the manifest (well, has no manifest at all).
CI-Examples/ra-tls-mbedtls/README.md line 170 at r2 (raw file):
make clean RA_TYPE=epid RA_CLIENT_SPID=<your SPID> RA_CLIENT_LINKABLE=<1 if SPID is linkable, else 0> \ make app client_epid.manifest.sgx
FYI: This was a bug in our README. Noticed it while testing.
CI-Examples/ra-tls-mbedtls/README.md line 172 at r3 (raw file):
gramine-sgx ./server & RA_TLS_ALLOW_DEBUG_ENCLAVE_INSECURE=1 RA_TLS_ALLOW_OUTDATED_TCB_INSECURE=1 \
FYI: This was a bug in our README. Noticed it while testing.
CI-Examples/ra-tls-mbedtls/src/server.c line 71 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Could we have the usual return code convention?
Done. Is that what you meant?
CI-Examples/ra-tls-mbedtls/src/server.c line 135 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
You could just pass
sizeof() - 1tofile_readinstead. This version handlesret == 0incorrectly.
Done
CI-Examples/ra-tls-mbedtls/src/server.c line 139 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
just
0please
Done. Not relevant anymore, since I removed \n from the output of /dev/attestation_attestation_type.
CI-Examples/ra-tls-mbedtls/src/server.c line 141 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Wouldn't it be easier to just throw
argvaway?
Done.
Documentation/attestation.rst line 230 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Maybe
unknown?unclearsounds weird to me
Done, not relevant anymore. unclear/unknown cannot be the content of this file anymore.
Documentation/attestation.rst line 232 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
If we expect everybody to strip the newline, then why we provide it?
Done.
Documentation/manifest-syntax.rst line 739 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
The glossary is a bit Gramine-independent, I think this stuff should rather stay here (your quote is purely "how to configure Gramine to do XYZ" and not an explanation of some SGX concept).
Done. No hurt in adding links to the terminology.
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I would remove it. I would expect most of the usages of this file to be programmed and they would need to remove this newline. In
shyou can docat file ; echoif you want a newline.
Done.
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd be fine with a shorter grace period.
I'm keeping as-is for now, until instructed otherwise.
LibOS/shim/src/fs/dev/attestation.c line 415 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
¬ needed
Done.
LibOS/shim/src/fs/dev/attestation.c line 448 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Could you just move this to the first usage? There is only one anyway.
Done.
Pal/src/host/Linux-SGX/common_parser.c line 45 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Hmm. So a bad type will list the possible options, but an unexpected string will just say "Unknown 'sgx.remote_attestation' type". I think it would make more sense if the same error message was emitted.
Done.
Pal/src/host/Linux-SGX/sgx_main.c line 758 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think the current behavior is fine (to allow this) for debugging purposes. But if it turns out that it confuses users then we should change it.
Ok, I'm keeping as-is until intstructed otherwise.
python/graminelibos/sgx_sign.py line 496 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I think the previous output was less confusing. This one kinda signals that
spidandlinkablematter even fordcapattestation, which is not the case.
Done. Slightly refactored for readability.
There was a problem hiding this comment.
Reviewed 2 of 29 files at r1.
Reviewable status: 24 of 35 files reviewed, 37 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I would remove it. I would expect most of the usages of this file to be programmed and they would need to remove this newline. In
shyou can docat file ; echoif you want a newline.
No, it should have newline at the end, like all files. If you don't want it, you can strip it.
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd be fine with a shorter grace period.
Nope, this 2 version delay is against our upstream dependencies (like, we support one Ubuntu version and the one before it).
In thing we support (for our downstream), we should support all deprecated syntax before releasing an deliberately incompatible version (like gramine 2.0, or if we think minor 1.x versions can be incompatible, another 1.x version). This is to prevent people from updating a minor version believing nothing incompatible has changed, and then oops, my app was broken by a version that promised to be compatible by doing only a minor release.
Those are two different policies, and we should have them documented somewhere.
Pal/src/host/Linux-SGX/common_parser.c line 33 at r2 (raw file):
} else if (!strcmp(sgx_attestation_type_str, "dcap")) { attestation_type = SGX_ATTESTATION_DCAP; } else {
Re: RFC about Azure schema: can you remind me if you really need this other Azure schema being distinct from dcap? If so do you think you could add else if (!strncmp(sgx_attestation_type, "x-", 2)) here to add arbitrary attestation schemas (being written like "x-maa")? Would that work?
(other option, we chan change dcap to strncmp(..., 4) if that's also sufficient for our internal logic and having those verify attestation schemas named "dcap-maa" and libs verify_dcap_maa`)
I'm trying to avoid adding arbitrary schemas here for every single Honest Achmed and his cloud.
There was a problem hiding this comment.
Reviewable status: 24 of 35 files reviewed, 37 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
Documentation/tutorials/pytorch/index.rst line 498 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: Please note that this PyTorch tutorial works only with DCAP attestation, so I hard-code it here.
Please add a comment about this.
There was a problem hiding this comment.
Reviewable status: 24 of 35 files reviewed, 37 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
CI-Examples/ra-tls-mbedtls/client.manifest.template line 8 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: This
/usr/local/libis needed because the in-SGX-enclave DCAP version of the client (client_dcap) is looking forlibdcap_quoteverify.solib which is installed under this directory, at least on Ubuntu.
This library could use pkg-config file. We could discover this library in runtime.
But I guess this is OK, because it's not really packaged in RPM (I mean, there's some offline repo which we can't use in practice), so we can hardcode deb-only things and wait for people to complain.
There was a problem hiding this comment.
Reviewed 11 of 11 files at r3, 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 (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @NieDzejkob, and @woju)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What do you want exactly? Do you want to move
RA_TYPEto other Jenkinsfiles (close to invocationsmake SGX=1 ...) under.ci/, to make the commands with which we build and run CI tests more explicit?
I want to have it inside Makefiles in CI-examples as an argument to manifest generation instead of env variable.
Unblocking though, maybe it's just subjective opinion.
CI-Examples/ra-tls-mbedtls/README.md line 73 at r3 (raw file):
an [SPID and the corresponding IAS API keys][spid]. [spid]: https://gramine.readthedocs.io/en/stable/sgx-intro.html#term-spid
I have mixed feelings here. On the one hand people installing from apt and cloning the repo just for examples want to have stable, but people just cloning the repo and building want to use latest...
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
like all files.
What files? This is devtmpfs.
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
my app was broken by a version that promised to be compatible by doing only a minor release.
We never promised that.
Pal/src/host/Linux-SGX/common_parser.c line 45 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
Not done?
There was a problem hiding this comment.
Reviewable status: 29 of 35 files reviewed, 24 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 @boryspoplawski, @NieDzejkob, and @woju)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
argument to manifest generation
I still don't understand you. Could you give an example?
CI-Examples/ra-tls-mbedtls/client.manifest.template line 8 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This library could use
pkg-configfile. We could discover this library in runtime.But I guess this is OK, because it's not really packaged in RPM (I mean, there's some offline repo which we can't use in practice), so we can hardcode deb-only things and wait for people to complain.
Ok, let's wait for people to complain :)
CI-Examples/ra-tls-mbedtls/README.md line 73 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I have mixed feelings here. On the one hand people installing from
aptand cloning the repo just for examples want to havestable, but people just cloning the repo and building want to uselatest...
Mixed feelings here as well. But I think it's more obvious for power users (they will recognize they are looking at stable and will replace it with latest), whereas the alternative would be confusing for novice users.
Documentation/tutorials/pytorch/index.rst line 498 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Please add a comment about this.
Done
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
like all files.
What files? This is devtmpfs.
I'll let Borys and Woju fight over this :)
LibOS/shim/src/fs/dev/attestation.c line 393 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Can't we just use
g_pal_public_state->attestation_typedirectly? Why the copying?
Done.
Pal/include/pal/pal.h line 87 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Is it a good idea to put PAL specific info here?
Well, we need to somehow learn the attestation type (none, EPID, DCAP). Surely we don't want to introduce a new Dk function for this.
Also, this can be considered not PAL specific. If we think of Gramine supporting e.g. Intel TDX or AMD SEV in the future, then we would need to introduce this variable anyway.
The variable is a string (instead of an enum) to hide the PAL specific details -- LibOS doesn't need to deal with it, LibOS only needs to provide this string to the application on top (e.g., to our RA-TLS library). Also, this is similar to host_type.
Pal/src/host/Linux-SGX/common_parser.c line 1 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This file needs a better name (to include it parses sgx specific manifest options).
common_sgx_parser.c?
Pal/src/host/Linux-SGX/common_parser.c line 33 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Re: RFC about Azure schema: can you remind me if you really need this other Azure schema being distinct from
dcap? If so do you think you could addelse if (!strncmp(sgx_attestation_type, "x-", 2))here to add arbitrary attestation schemas (being written like"x-maa")? Would that work?(other option, we chan change
dcaptostrncmp(..., 4)if that's also sufficient for our internal logic and having those verify attestation schemas named"dcap-maa" and libsverify_dcap_maa`)I'm trying to avoid adding arbitrary schemas here for every single Honest Achmed and his cloud.
Actually, we won't need anything other than none/epid/dcap (at least in the foreseable future).
I was wrong about the need for maa in here. MAA only needs to be known by RA-TLS, which sits on top of Gramine. The core Gramine doesn't need to know about MAA. I will update the issue #626
So yeah, there will be no arbitrary schemas.
Pal/src/host/Linux-SGX/common_parser.c line 35 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This function should return PAL error codes
Done. Actually I'm keeping UNIX error codes here, and changing PAL error codes to UNIXes below. See also my top-level comment about why I do so.
Pal/src/host/Linux-SGX/common_parser.c line 43 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
wrong alignment
Done.
Pal/src/host/Linux-SGX/common_parser.c line 45 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Not done?
This was me prematurely hitting the Publish button. Now done.
Pal/src/host/Linux-SGX/common_parser.c line 47 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto (PAL error codes)
Done.
Pal/src/host/Linux-SGX/common_parser.c line 59 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto (PAL error codes)
Done.
Pal/src/host/Linux-SGX/common_parser.c line 61 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
AFAICS we could parse this first, before the attestation type itself. Then you could put the logic to distinguish the legacy syntax where the
attestation_type = SGX_ATTESTATION_UNCLEARline currently is, and refactor out this enum variant entirely.I think we also have somewhat inconsistent behavior here: if the attestation type is not epid, then a wrong type for
ra_client_linkablewill be ignored, butra_client_spidwill not. I don't think supportingsgx.ra_client_spid = []or something makes sense anyway, but we should be consistent in detecting that for each parameter.
Done.
Pal/src/host/Linux-SGX/common_parser.c line 64 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This seem to be the only usage of
SGX_ATTESTATION_UNCLEARnow, so we should get rid of it.
Done.
Pal/src/host/Linux-SGX/common_parser.c line 93 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Why is this done by both
parse_attestation_epid_paramsandparse_attestation_type?
Because we have a weird legacy syntax to determine parse_attestation_type():
sgx.remote_attestation = true
sgx.ra_client_spid = "<something>" # Gramine recognizes this combination as EPID
vs
sgx.remote_attestation = true
sgx.ra_client_spid = "" # Gramine recognizes this combination as DCAP
We have to support this legacy syntax for a while, so that's why I have this code snippet in parse_attestation_type(). I will remove this code snippet about sgx.ra_client_spid from this func as soon as we completely remove the support for this legacy syntax.
Pal/src/host/Linux-SGX/common_parser.c line 111 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
We already have
parse_hexinPal/src/host/Linux-SGX/tools/common/util.c, perhaps it would be a good idea to move that tocommon/instead of duplicating an ad-hoc implementation here?
I looked into it, and it's not that easy. The util.c file assumes a proper Glibc environment, so it won't be able to use the common/api.h include anyway. We can still have some helper function for core Gramine code itself (I found several code snippets like this by grepping hex2dec), but I suggest to keep this out of scope of this PR.
I added this to my TODO list, I'll prepare a PR (or if anyone else wants, feel free to do it).
Pal/src/host/Linux-SGX/db_main.c line 635 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Do we really want to make a stringly-typed variable a part of our public interface? This reeks of JavaScript. Why not store the enum value itself in
g_pal_public_state?
As mentioned in another comment, this is actually opaque to the LibOS component. LibOS doesn't care what attestation type the underlying PAL uses. What might care about this is the application on top. That's why I choose a string with arbitrary contents instead of a proper enum.
But if people have better ideas how to make it clean and hide it from LibOS at the same time, suggestions are welcome.
Pal/src/host/Linux-SGX/sgx_internal.h line 44 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Yes please
Done.
Pal/src/host/Linux-SGX/sgx_main.c line 734 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Will need to translate PAL -> UNIX errno here.
We don't have such a function. But we have unix_to_pal_error(), so that's what I did (flipped your logic).
There was a problem hiding this comment.
Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 17 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 @boryspoplawski, @dimakuv, @NieDzejkob, and @woju)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
argument to manifest generation
I still don't understand you. Could you give an example?
Something like:
gramine-manifest \
-Dlog_level=$(GRAMINE_LOG_LEVEL) \
-Darch_libdir=$(ARCH_LIBDIR) \
-Dattest_scheme=$(ATTEST_SCHEME)
and then in .ci/ (as you described above) make SGX=1 ATTEST_SCHEME=epid
LibOS/shim/src/fs/dev/attestation.c line 393 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
Then why do we need this variable at all? :)
Pal/src/host/Linux-SGX/common_parser.c line 1 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
common_sgx_parser.c?
common_manifest_sgx_parser.c?
There was a problem hiding this comment.
Reviewed 1 of 6 files at r4.
Reviewable status: all files reviewed, 16 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 @boryspoplawski, @dimakuv, and @NieDzejkob)
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'll let Borys and Woju fight over this :)
This is a text file which people will be cating from inside the enclave. All text-like pseudofiles have a newline at the end. If you don't want a newline, I guess you can have uint32_t here.
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
my app was broken by a version that promised to be compatible by doing only a minor release.
We never promised that.
Like I said, we need to document what exactly we promise (and expect from other projects), so people have a common understanding. If we don't, people might draw wrong conclusions from our version numbers.
Pal/src/host/Linux-SGX/common_parser.c line 33 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, we won't need anything other than none/epid/dcap (at least in the foreseable future).
I was wrong about the need for
maain here. MAA only needs to be known by RA-TLS, which sits on top of Gramine. The core Gramine doesn't need to know about MAA. I will update the issue #626So yeah, there will be no arbitrary schemas.
OK, so keeping the set of valid values closed is good.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 16 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 @boryspoplawski, @dimakuv, @NieDzejkob, and @woju)
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This is a text file which people will be
cating from inside the enclave. All text-like pseudofiles have a newline at the end. If you don't want a newline, I guess you can haveuint32_there.
This is a file people will read programmatically in their attestation wrappers, not cat from shell. uint32_t would be fine, but then we would need to introduce an enum for this, publish as part of API and keep backward compatibility. This is just easier.
There was a problem hiding this comment.
Reviewed 7 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 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 @boryspoplawski, @dimakuv, and @woju)
CI-Examples/busybox/README.md line 31 at r4 (raw file):
[spid]: https://gramine.readthedocs.io/en/stable/sgx-intro.html#term-spid [attestation]: https://gramine.readthedocs.io/en/stable/attestation.html
I guess we could move the [attestation] link to be closer to where we refer to it in text? That way it's easier to follow the link when you're reading the un-rendered markdown.
CI-Examples/ra-tls-mbedtls/README.md line 73 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Mixed feelings here as well. But I think it's more obvious for power users (they will recognize they are looking at
stableand will replace it withlatest), whereas the alternative would be confusing for novice users.
I think that sgx-intro in particular is not something that is particularly dependent on the Gramine version, so we'd be better off linking to latest here, so that any wording improvements we merge in the future, for example, will be referenced here without waiting for a release.
Pal/src/host/Linux-SGX/common_parser.c line 45 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This was me prematurely hitting the Publish button. Now done.
It's not ideal that the string itself is now duplicated, but I won't insist on cleaning that up.
Pal/src/host/Linux-SGX/common_parser.c line 93 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Because we have a weird legacy syntax to determine
parse_attestation_type():sgx.remote_attestation = true sgx.ra_client_spid = "<something>" # Gramine recognizes this combination as EPIDvs
sgx.remote_attestation = true sgx.ra_client_spid = "" # Gramine recognizes this combination as DCAPWe have to support this legacy syntax for a while, so that's why I have this code snippet in
parse_attestation_type(). I will remove this code snippet aboutsgx.ra_client_spidfrom this func as soon as we completely remove the support for this legacy syntax.
Hmm. Though right now, we can just have one function that parses these three relevant manifest keys all at once, avoiding any code duplication.
Pal/src/host/Linux-SGX/common_parser.c line 111 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I looked into it, and it's not that easy. The
util.cfile assumes a proper Glibc environment, so it won't be able to use thecommon/api.hinclude anyway. We can still have some helper function for core Gramine code itself (I found several code snippets like this by greppinghex2dec), but I suggest to keep this out of scope of this PR.I added this to my TODO list, I'll prepare a PR (or if anyone else wants, feel free to do it).
Okay, unblocking then.
Pal/src/host/Linux-SGX/db_main.c line 635 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
As mentioned in another comment, this is actually opaque to the LibOS component. LibOS doesn't care what attestation type the underlying PAL uses. What might care about this is the application on top. That's why I choose a string with arbitrary contents instead of a proper enum.
But if people have better ideas how to make it clean and hide it from LibOS at the same time, suggestions are welcome.
Okay. If the only place this string is touched is to expose it in /dev, I suppose it does make sense.
python/graminelibos/sgx_sign.py line 497 at r4 (raw file):
elif attestation_type == "dcap": print(' DCAP/ECDSA') else:
I'd explicitly do elif attestation_type == "epid" and have an else that handles the possibility of an unknown setting here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 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 @boryspoplawski, @dimakuv, and @woju)
Documentation/manifest-syntax.rst line 739 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. No hurt in adding links to the terminology.
Ah, I misread the comment. I thought you want to extend the glossary by the quote you pasted.
Linking there from here of course is a good idea.
There was a problem hiding this comment.
Reviewable status: 28 of 35 files reviewed, 8 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 @boryspoplawski, @NieDzejkob, and @woju)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Something like:
gramine-manifest \ -Dlog_level=$(GRAMINE_LOG_LEVEL) \ -Darch_libdir=$(ARCH_LIBDIR) \ -Dattest_scheme=$(ATTEST_SCHEME)and then in
.ci/(as you described above)make SGX=1 ATTEST_SCHEME=epid
Sorry for dragging this, but why is this helpful? We're just adding one more level of indirection (we introduce an envvar ATTEST_SCHEME that is transformed into a Jinja var attest_scheme), but we don't solve your original problem: one can still set the variable ATTEST_SCHEME=epid and forget about it. How does your proposal make it better / more explicit? To me, it just adds one more level of indirection to deal with (Makefiles).
CI-Examples/busybox/README.md line 31 at r4 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I guess we could move the
[attestation]link to be closer to where we refer to it in text? That way it's easier to follow the link when you're reading the un-rendered markdown.
Done.
CI-Examples/ra-tls-mbedtls/README.md line 73 at r3 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I think that sgx-intro in particular is not something that is particularly dependent on the Gramine version, so we'd be better off linking to
latesthere, so that any wording improvements we merge in the future, for example, will be referenced here without waiting for a release.
Ok, done. latest everywhere.
LibOS/shim/src/fs/dev/attestation.c line 393 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Then why do we need this variable at all? :)
Done.
python/graminelibos/sgx_sign.py line 497 at r4 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I'd explicitly do
elif attestation_type == "epid"and have anelsethat handles the possibility of an unknown setting here.
Done.
Pal/src/host/Linux-SGX/common_parser.c line 1 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
common_manifest_sgx_parser.c?
Done.
Pal/src/host/Linux-SGX/common_parser.c line 93 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Hmm. Though right now, we can just have one function that parses these three relevant manifest keys all at once, avoiding any code duplication.
Done. You are right, it actually looks better now.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 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 @dimakuv, @NieDzejkob, and @woju)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
Sorry for dragging this, but why is this helpful?
Sure, I'm not sure if it is.
we're just adding one more level of indirection (we introduce an envvar ATTEST_SCHEME that is transformed into a Jinja var attest_scheme),
I just wanted to make this an argument to make instead of a env var, but well, with make they are pretty much the same.
The only difference would be how manifests takes it - I'm proposing it to be an argument (to gramine-manifest), rather than envvar
Pal/src/host/Linux-SGX/common_manifest_sgx_parser.c line 55 at r5 (raw file):
ret = get_epid_params(manifest_root, &sgx_ra_client_spid_str, &dummy_linkable); if (ret < 0) { ret = -EINVAL;
Why not forward the already set ret?
Pal/src/host/Linux-SGX/common_manifest_sgx_parser.c line 115 at r5 (raw file):
ret = get_epid_params(manifest_root, &sgx_ra_client_spid_str, &linkable); if (ret < 0) { ret = -EINVAL;
ditto
There was a problem hiding this comment.
Reviewable status: 34 of 35 files reviewed, 9 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 @boryspoplawski, @NieDzejkob, and @woju)
-- commits line 9 at r6:
Blocking myself:
- Remove mention of MAA, we won't be introducing
sgx.remote_attestation = "maa"(MAA will use"dcap") - Rename from
remote attestation schemestoSGX quote formats, because this is a more correct term here
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Sorry for dragging this, but why is this helpful?
Sure, I'm not sure if it is.
we're just adding one more level of indirection (we introduce an envvar ATTEST_SCHEME that is transformed into a Jinja var attest_scheme),
I just wanted to make this an argument to
makeinstead of a env var, but well, with make they are pretty much the same.
The only difference would be how manifests takes it - I'm proposing it to be an argument (togramine-manifest), rather than envvar
Hm, I still don't see how this helps with your initial complaint about This might get annoying if you set this variable and forget about it. Adding an argument to make doesn't solve your initial complaint -- one can still set this variable (ATTEST_SCHEME) in env and forget about it.
I'm still reluctant to introduce such a change.
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This is a file people will read programmatically in their attestation wrappers, not
catfrom shell.uint32_twould be fine, but then we would need to introduce an enum for this, publish as part of API and keep backward compatibility. This is just easier.
I agree with Borys. This file is under /dev/, where files are not supposed to be human-readable. Also, all other files under /dev/attestation/ are not in human-readable form (almost all of them are actually binary blobs).
So I actually like the current version -- just a string like dcap\0, without a newline. It is easier to work with (see ra-tls-mbedtls example).
LibOS/shim/src/fs/dev/attestation.c line 397 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Like I said, we need to document what exactly we promise (and expect from other projects), so people have a common understanding. If we don't, people might draw wrong conclusions from our version numbers.
I agree with Woju, we need to agree on the policy and document it.
Pal/src/host/Linux-SGX/common_manifest_sgx_parser.c line 55 at r5 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why not forward the already set
ret?
Done.
Pal/src/host/Linux-SGX/common_manifest_sgx_parser.c line 115 at r5 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @NieDzejkob and @woju)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
ATTEST_SCHEME == RA_TYPE in my example, idk why I used a different name.
The point is that in this approach you have to set it by env var - there is no other option. What I proposed is using it by args to make - you have to set them each time, so they won't pollute your env and you cannot just forget about it.
I'm still reluctant to introduce such a change.
All right, not insisting.
There was a problem hiding this comment.
Reviewed 12 of 29 files at r1, 2 of 11 files at r2, 6 of 11 files at r3, 5 of 7 files at r5.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @NieDzejkob)
CI-Examples/busybox/README.md line 22 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
I'm sorry for being late here, but can we switch it the other way around and have make first? If there are too many or too long variables to the point of line wrap, I think the command should go first, otherwise it looks bad like here.
CI-Examples/python/scripts/sgx-quote.py line 13 at r6 (raw file):
"are you running under SGX, with remote attestation enabled?") sys.exit(1)
(NB) Although it doesn't change anything in the quote, do you think it would be convenient to add here something like print(f"Detected attestation type: {open('/dev/attestation/type').read()}"), just to make user aware?
LibOS/shim/src/fs/dev/attestation.c line 252 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree with Borys. This file is under
/dev/, where files are not supposed to be human-readable. Also, all other files under/dev/attestation/are not in human-readable form (almost all of them are actually binary blobs).So I actually like the current version -- just a string like
dcap\0, without a newline. It is easier to work with (seera-tls-mbedtlsexample).
OK.
Pal/src/host/Linux-SGX/meson.build line 55 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: I wasn't sure where to put the file that is common only to untrusted PAL and trusted PAL. So I just added it in both lists explicitly. Any better ideas?
You could add another variable, but IIUC it would hold only a single file. So if you think there will be more of such files, then I'd say you can add such a variable, and if not, then I'm not sure it's worth it. 'common_*' prefix looks clear enough.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @NieDzejkob)
LibOS/shim/src/fs/dev/attestation.c line 255 at r8 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Hmm, I think Dmitrii did this on purpose - the output data should not contain the trailing nullbyte, so it's actually the data size?
Depending how you look at it. out_data contains a pointer to a C string (correctly null terminated). But I guess callers are not interested in this information.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow and @NieDzejkob)
Previously, mkow (Michał Kowalczyk) wrote…
Isn't it more like a scheme? E.g. EPID uses IAS, but DCAP doesn't.
What is more like a scheme?
I use the following terminology:
- SGX quote format -- the format of the SGX quote generated by the Quoting Enclave on the same machine. Currently Intel introduced only two formats -- an EPID quote and a DCAP quote (they differ in some fields, and importantly they differ in the
versioninteger field, see ) - SGX attestation scheme -- the flow how the generated SGX quote is verified by a remote party. Confusingly, Intel chose the same words for the attestation schemes as for SGX quote formats -- EPID and DCAP. EPID should be more correctly called "IAS-based" (Intel Attestation Service). DCAP should be more correctly called "PCS-based" (Provisioning Certificates Service). But what's done is done. Now, in addition to those two schemes, Microsoft introduces a new scheme called "MAA-based" (Microsoft Azure Attestation).
So my comment seems correct -- this PR and the manifest option sgx.remote_attestation are about SGX quote formats.
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
So, would be good to be uniform in this - I really don't like that we pass some arguments to
gramine-manifestthrough env, and some through-D, and the split seems accidental.
I don't think the split is accidental. Basically, the rule is like this:
- If something can be passed as an environment variable, then pass it as an environment variable directly.
- If not (e.g., the
entrypointis typically detected via smth likewhich python3), then pass it through-D.
I think this is a reasonable rule. It removes the bloat/one indirection level.
LibOS/shim/src/fs/dev/attestation.c line 255 at r8 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Depending how you look at it.
out_datacontains a pointer to a C string (correctly null terminated). But I guess callers are not interested in this information.
This attestation_type_load() is a callback. The function-pointer type for this callback has the third argument out_size, so this was the only reason why I used out_size here :) Anyway, seems like a resolved discussion.
Pal/src/host/Linux-SGX/common_manifest_sgx_parser.c line 74 at r8 (raw file):
} } else { /* TODO: Bool syntax is deprecated in v1.3, remove 2 versions later. */
FYI: @mkow I have the deprecation comment here.
Pal/src/host/Linux-SGX/common_parser.c line 93 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I will remove this code snippet about sgx.ra_client_spid from this func as soon as we completely remove the support for this legacy syntax.
@dimakuv: Please add a TODO with a "version counter" (i.e. something like "Deprecated, drop/simplify X releases after Y.Z")
@mkow Add a TODO where? I already have one TODO, see my other comment. Where else do we want to put it?
(Please note that you quoted my reply from an old version of the code. After this quote, I actually modified the code according to @niedzejkob's request.)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @NieDzejkob)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is more like a scheme?
I use the following terminology:
- SGX quote format -- the format of the SGX quote generated by the Quoting Enclave on the same machine. Currently Intel introduced only two formats -- an EPID quote and a DCAP quote (they differ in some fields, and importantly they differ in the
versioninteger field, see )- SGX attestation scheme -- the flow how the generated SGX quote is verified by a remote party. Confusingly, Intel chose the same words for the attestation schemes as for SGX quote formats -- EPID and DCAP. EPID should be more correctly called "IAS-based" (Intel Attestation Service). DCAP should be more correctly called "PCS-based" (Provisioning Certificates Service). But what's done is done. Now, in addition to those two schemes, Microsoft introduces a new scheme called "MAA-based" (Microsoft Azure Attestation).
So my comment seems correct -- this PR and the manifest option
sgx.remote_attestationare about SGX quote formats.
Oh, I see. Could you create a PR adding this explanation about quote formats to our docs? This is super confusing...
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
If something can be passed as an environment variable, then pass it as an environment variable directly.
What about log level? It could also be an env variable, but we pass it through -D?
It removes the bloat/one indirection level.
It's not much code (just one additional -D line pasting an env there?), so I'd prefer to be consistent with other things and just parametrize everything in manifests through -D and have one place where you can see all the arguments for manifest generation (without anything hidden in the environment). Despite it's longer, it feels simpler and more straightforward to me.
LibOS/shim/src/fs/dev/attestation.c line 255 at r8 (raw file):
Depending how you look at it.
out_datacontains a pointer to a C string (correctly null terminated). But I guess callers are not interested in this information.
I rather read this as an over-allocated (for convenience) buffer. The caller is not allowed to assume that it's longer than *out_size.
Pal/src/host/Linux-SGX/common_parser.c line 93 at r2 (raw file):
Please note that you quoted my reply from an old version of the code. After this quote, I actually modified the code according to @NieDzejkob's request.
Ah, ok, resolving then.
There was a problem hiding this comment.
Reviewed 1 of 29 files at r1, 2 of 11 files at r2, 3 of 11 files at r3, 4 of 6 files at r4, 2 of 7 files at r5, 1 of 1 files at r6, 4 of 5 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
There was a problem hiding this comment.
Reviewable status: 26 of 39 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 @boryspoplawski, @mkow, @NieDzejkob, and @woju)
Previously, mkow (Michał Kowalczyk) wrote…
Oh, I see. Could you create a PR adding this explanation about quote formats to our docs? This is super confusing...
@mkow Currently created a GitHub issue: #654, see item 1.
.ci/lib/stage-test-sgx.jenkinsfile line 122 at r10 (raw file):
if [ "${ra_client_spid}" != "" ] && [ "${ra_client_key}" != "" ]; \ then \ make SGX=1 check_epid RA_CLIENT_SPID=${ra_client_spid} \
FYI: SGX=1 is not needed for this example -- it always assumes SGX.
LibOS/shim/test/regression/attestation_deprecated_syntax.manifest.template line 28 at r8 (raw file):
sgx.remote_attestation = true sgx.ra_client_spid = "{{ env.get('RA_CLIENT_SPID', '') }}" sgx.ra_client_linkable = {{ 'true' if env.get('RA_CLIENT_LINKABLE', '0') == '1' else 'false' }}
FYI: I kept the syntax of env.get() here because these are regression tests, and there is no simple Makefile to tweak (the tweaking happens in gramine-test tool)
.ci/lib/config.jenkinsfile line 10 at r1 (raw file):
Done.
I fixed the occurences touched in this PR, and created a GitHub issue #654 (item 2) to fix in other places in a separate PR, if any.
and then in
.ci/(as you described above)make SGX=1 RA_TYPE=epid
Also done.
There was a problem hiding this comment.
Reviewed 10 of 10 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 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 and @mkow)
CI-Examples/busybox/busybox.manifest.template line 25 at r10 (raw file):
sgx.debug = true sgx.remote_attestation = "{{ ra_type }}"
Why do we even have RA in this example? Do we test it anywhere?
LibOS/shim/test/regression/attestation_deprecated_syntax.manifest.template line 28 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: I kept the syntax of
env.get()here because these are regression tests, and there is no simple Makefile to tweak (the tweaking happens ingramine-testtool)
Why can't we use default value here instead of Makefile? You can use jinja default filter, for example.
There was a problem hiding this comment.
Reviewed 10 of 10 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 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 and @mkow)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 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 @boryspoplawski and @mkow)
CI-Examples/busybox/busybox.manifest.template line 25 at r10 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why do we even have RA in this example? Do we test it anywhere?
We don't test it in CI (well, the CI build this example with sgx.remote_attestation = "epid" but nothing is really tested). This is purely for convenience of users -- e.g., I frequently use busybox to test these pseudo-files under /dev/attestation. I find it nice to mention this, see also my comment in the README file.
CI-Examples/busybox/README.md line 10 at r10 (raw file):
If you want to try out [`/dev/attestation/` files][attestation], you must build the example with SGX remote attestation enabled. By default, the example is built *without* remote attestation.
FYI: @boryspoplawski This is why sgx.remote_attestation is useful in this example. Just for testing.
LibOS/shim/test/regression/attestation_deprecated_syntax.manifest.template line 28 at r8 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why can't we use default value here instead of Makefile? You can use jinja default filter, for example.
Which Makefile? We don't have a Makefile for LibOS regression tests. Or maybe I misunderstood you, then please expand on your suggestion.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 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 and @mkow)
LibOS/shim/test/regression/attestation_deprecated_syntax.manifest.template line 28 at r8 (raw file):
instead of Makefile
I meant directly in this manifest.template, opposite to other examples where we use Makefile for this.
please expand on your suggestion
sgx.ra_client_spid = "{{ ra_type|default('', true) }}"
Or something, writing from the top of my head
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow)
LibOS/shim/test/regression/attestation_deprecated_syntax.manifest.template line 28 at r8 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
instead of Makefile
I meant directly in this manifest.template, opposite to other examples where we use Makefile for this.
please expand on your suggestion
sgx.ra_client_spid = "{{ ra_type|default('', true) }}"Or something, writing from the top of my head
Sorry, I've misunderstood the problem here.
The current version is fine
There was a problem hiding this comment.
Reviewed 4 of 11 files at r3, 1 of 6 files at r4, 4 of 5 files at r7, 1 of 2 files at r8, 10 of 10 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
CI-Examples/busybox/README.md line 38 at r10 (raw file):
```sh # build Busybox and the final manifest SGX=1 make
Why this change?
CI-Examples/ra-tls-mbedtls/src/server.c line 154 at r10 (raw file):
if (ra_tls_attest_lib) { mbedtls_printf("\n . Creating the RA-TLS server cert and key (using %s)...",
(otherwise you can end up with confusing messages like "Creating the RA-TLS server cert and key (using none)" shown to the user)
Suggestion:
using "%s" as attestation modeDocumentation/attestation.rst line 225 at r10 (raw file):
(``/dev/attestation/keys/<key_name>``) use a 16-byte raw binary value. Finally, the ``/dev/attestation`` pseudo-filesystem exposes a pseudo-file that
Wouldn't this fit better into the list of /dev/attestation entries above?
There was a problem hiding this comment.
Reviewed 4 of 29 files at r1, 4 of 11 files at r2, 3 of 6 files at r4, 1 of 7 files at r5, 1 of 1 files at r6, 1 of 2 files at r8.
Reviewable status: all files reviewed, 8 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
LibOS/shim/src/fs/dev/attestation.c line 406 at r10 (raw file):
log_debug("host is Linux-SGX and remote attestation type is '%s', adding SGX-specific " "/dev/attestation files: report, quote, etc.", g_pal_public_state->attestation_type);
Suggestion:
/dev/attestation/ filesLibOS/shim/src/fs/dev/attestation.c line 422 at r10 (raw file):
/* TODO: This file is deprecated in v1.2, remove 2 versions later. */ struct pseudo_node* deprecated_pfkey = pseudo_add_str(attestation, "protected_files_key", &deprecated_pfkey_load);
please fix wrapping
Pal/src/host/Linux-SGX/db_misc.c line 549 at r10 (raw file):
ret = parse_attestation_type(g_pal_public_state.manifest_root, &attestation_type); if (ret < 0) { /* error is already printed by the called func */
Suggestion:
was already printedPal/src/host/Linux-SGX/db_misc.c line 556 at r10 (raw file):
ret = parse_attestation_epid_params(g_pal_public_state.manifest_root, &spid, &linkable); if (ret < 0) { /* error is already printed by the called func */
ditto
Pal/src/host/Linux-SGX/sgx_main.c line 960 at r10 (raw file):
/* initialize communication with Quoting Enclave only if app requests attestation */ bool is_epid = enclave->attestation_type == SGX_ATTESTATION_EPID; log_debug("Using SGX %s quotes for remote attestation", is_epid ? "EPID" : "DCAP/ECDSA");
Why not just print enclave->attestation_type directly?
There was a problem hiding this comment.
Reviewable status: 31 of 39 files reviewed, 8 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 @boryspoplawski, @mkow, @NieDzejkob, and @woju)
CI-Examples/busybox/README.md line 38 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why this change?
Done. We had a back-and-forth about where to put envvars, and looks like I forgot to revert this change (once we settled on "put envvars after make").
CI-Examples/ra-tls-mbedtls/src/server.c line 154 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
(otherwise you can end up with confusing messages like "Creating the RA-TLS server cert and key (using none)" shown to the user)
Done. But replaced mode -> type, to conform to the other places (in particular, the file name /dev/attestation/attestation_type)
Documentation/attestation.rst line 225 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Wouldn't this fit better into the list of
/dev/attestationentries above?
Done.
LibOS/shim/src/fs/dev/attestation.c line 406 at r10 (raw file):
log_debug("host is Linux-SGX and remote attestation type is '%s', adding SGX-specific " "/dev/attestation files: report, quote, etc.", g_pal_public_state->attestation_type);
Done.
LibOS/shim/src/fs/dev/attestation.c line 422 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
please fix wrapping
Done.
Can this auto-wrapping rule be fixed in Vim? I just use gq to re-wrap, and then manually re-wrap to our guidelines. Is there a better way?
Pal/src/host/Linux-SGX/db_misc.c line 549 at r10 (raw file):
ret = parse_attestation_type(g_pal_public_state.manifest_root, &attestation_type); if (ret < 0) { /* error is already printed by the called func */
Done.
Pal/src/host/Linux-SGX/db_misc.c line 556 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
Pal/src/host/Linux-SGX/sgx_main.c line 960 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why not just print
enclave->attestation_typedirectly?
Done.
There was a problem hiding this comment.
Reviewed 8 of 8 files at r11, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow)
CI-Examples/busybox/README.md line 38 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. We had a back-and-forth about where to put envvars, and looks like I forgot to revert this change (once we settled on "put envvars after make").
Technically after make they are arguments to make, not envvars :) (which I guess is what we want here)
LibOS/shim/src/fs/dev/attestation.c line 422 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
Can this auto-wrapping rule be fixed in Vim? I just use
gqto re-wrap, and then manually re-wrap to our guidelines. Is there a better way?
If you are deleting whitespaces then most of the time you don't need to rewrap anything (and for deletion you can just delete in visual block mode). But the wrapping itself (if needed) I do manually (sometimes just set colorcolumn=101) :)
There was a problem hiding this comment.
Reviewed 8 of 8 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Documentation/attestation.rst line 121 at r11 (raw file):
below pseudo-files: - ``/dev/attestation/attestation_type`` pseudo-file can be opened for read and
Suggestion:
for reading
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
Documentation/attestation.rst line 121 at r11 (raw file):
below pseudo-files: - ``/dev/attestation/attestation_type`` pseudo-file can be opened for read and
Oh wait, this whole table and LibOS/src/fs/dev/attestation.c is written this way, let's leave the fix for another PR then, this one is already big.
mkow
force-pushed
the
dimakuv/add-dev-attestation-type
branch
from
a1f5bbe
to
d3eb46e
Compare
There was a problem hiding this comment.
Reviewed 15 of 15 files at r12, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
This commit deprecates ambiguous `sgx.remote_attestation = true` in favor of an explicit attestation-type string. The first side benefit of this change is that the weird logic "if ra_client_spid is set, then it is EPID otherwise DCAP" is gone. The second side benefit is that this allows us to add more SGX quote formats in the future, if needed. Also, a new pseudo-file `/dev/attestation/attestation_type` is introduced at the LibOS level. It helps the application to determine which attestation scheme should be used. Several CI examples are rewritten to use the new syntax: Python, ra-tls-mbedlts, ra-tls-secret-prov, Busybox (the latter is purely for testing purposes). The `attestation` LibOS regression test also uses the new syntax; the old syntax is tested via the newly added test `attestation_deprecated_syntax`. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
dimakuv
force-pushed
the
dimakuv/add-dev-attestation-type
branch
from
d3eb46e
to
5faa9eb
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
I verified this final rebase manually on two machines: EPID and DCAP. Everything works.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done (Michal forgot to fix this during rebase).
Description of the changes
This PR deprecates ambiguous
sgx.remote_attestation = truein favor of an explicit attestation-type string. The first side benefit of this change is that the weird logic "if ra_client_spid is set, then it is EPID otherwise DCAP" is gone. The second side benefit is that this allows us to add more remote attestation schemes in the future, if needed (for example, Microsoft Azure Attestation).Also, a new pseudo-file
/dev/attestation/attestation_typeis introduced at the LibOS level. It helps the application to determinewhich attestation scheme should be used.
Several CI examples are rewritten to use the new syntax: Python, ra-tls-mbedlts, ra-tls-secret-prov, Busybox (the latter is purely for testing purposes). The
attestationLibOS regression test also uses the new syntax; the old syntax is tested via the newly added testattestation_deprecated_syntax.How to test this PR?
CI and manually (e.g., look around via
gramine-sgx busybox sh).Testing on Azure CC VM
(Tried on Ubuntu 18.04.)
libsgx_dcap_quoteverify.sosymlink by default, so set up the symlink manually:I've done all the above steps and tested the examples in step 7 using both EPID and DCAP (the Azure CC VM allowed both). Everything works as expected.
This change is