[RA-TLS] Adapt for the new encrypted files API #587
Conversation
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NieDzejkob)
-- commits line 2 at r1:
We don't write RA-TLS as a separate component in our commits. I suggest:
[Pal/Linux-SGX] Adapt RA-TLS for the new encrypted files API
CI-Examples/ra-tls-secret-prov/README.md line 79 at r1 (raw file):
# test encrypted-files client gramine-sgx ./secret_prov_pf_client
We should rename _pf_ to _enc_files_ I guess.
We should also rename all those files in examples that have pf in their names (well, maybe it's only this secret_prov_pf_client).
Pal/src/host/Linux-SGX/tools/common/util.h line 100 at r1 (raw file):
* \param[in] buffer_size Size of the buffer. * \param[in] masked If non-NULL, this string will be used in error messages instead * of the input hexstring itself. Use when parsing sensitive data.
Why such weird alignment of arguments? For uniformity with other parts of the code, it should be like this:
* \param[in] buffer_size Size of the buffer.
Note the double-space after the arg name.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 334 at r1 (raw file):
if (e) { char keydata[16];
Instead of 16, you should be able to use sgx_key_128bit_t here (declared in sgx_arch.h which is included in ra_tls.h), and you can do sizeof(sgx_key_128bit_t) to get 16.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 340 at r1 (raw file):
char path_buf[256]; if (snprintf(path_buf, 256, "/dev/attestation/keys/%s", e) >= 256) { ERROR("Key name '%s' too long\n", path_buf);
But in this case, path_buf will be truncated and not null terminated, so you'll end up printing some garbage. Maybe change the error message to mention e instead of path_buf?
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 340 at r1 (raw file):
char path_buf[256]; if (snprintf(path_buf, 256, "/dev/attestation/keys/%s", e) >= 256) { ERROR("Key name '%s' too long\n", path_buf);
Also, we don't print errors in this file. But now that I'm thinking about it -- maybe we should. Otherwise debugging this file is hard (I use GDB, but with log messages it would be much better for end users).
So maybe you could add log messages to other parts in this function?
There was a problem hiding this comment.
Reviewable status: 7 of 9 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We don't write
RA-TLSas a separate component in our commits. I suggest:[Pal/Linux-SGX] Adapt RA-TLS for the new encrypted files API
Okay, TODO for rebase time.
CI-Examples/ra-tls-secret-prov/README.md line 79 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We should rename
_pf_to_enc_files_I guess.We should also rename all those files in examples that have
pfin their names (well, maybe it's only thissecret_prov_pf_client).
Considering the abbreviation is still alive in the name of gramine-sgx-pf-{crypt,tamper}, I wouldn't fret about removing it from examples at the moment.
(also it will conflict with #588)
Pal/src/host/Linux-SGX/tools/common/util.h line 100 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why such weird alignment of arguments? For uniformity with other parts of the code, it should be like this:
* \param[in] buffer_size Size of the buffer.Note the double-space after the arg name.
Fixed. I was looking at read_file above as an example, but apparently extrapolated wrong.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 334 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Instead of
16, you should be able to usesgx_key_128bit_there (declared insgx_arch.hwhich is included inra_tls.h), and you can dosizeof(sgx_key_128bit_t)to get 16.
Done.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 340 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But in this case,
path_bufwill be truncated and not null terminated, so you'll end up printing some garbage. Maybe change the error message to mentioneinstead ofpath_buf?
Done.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 340 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Also, we don't print errors in this file. But now that I'm thinking about it -- maybe we should. Otherwise debugging this file is hard (I use GDB, but with log messages it would be much better for end users).
So maybe you could add log messages to other parts in this function?
I think that's best left for another PR, especially considering that we might want this one merged for the impending release.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
CI-Examples/ra-tls-secret-prov/README.md line 79 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Considering the abbreviation is still alive in the name of
gramine-sgx-pf-{crypt,tamper}, I wouldn't fret about removing it from examples at the moment.(also it will conflict with #588)
OK, you're right.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 340 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I think that's best left for another PR, especially considering that we might want this one merged for the impending release.
Ok, fair enough.
|
Jenkins, test this please |
There was a problem hiding this comment.
Reviewed 7 of 9 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @NieDzejkob)
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Okay, TODO for rebase time.
Actually, maybe we should start doing it? It's technically separate from SGX PAL. Not blocking though.
Pal/src/host/Linux-SGX/tools/common/util.h line 104 at r2 (raw file):
const char* masked
I think it should be rather named mask.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 322 at r2 (raw file):
/* successfully retrieved the secret: is it a key for encrypted files? */ e = getenv(SECRET_PROVISION_SET_KEY);
Please use a more descriptive name (doesn't have to me long, just not a single random letter).
Code quote:
e
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
Pal/src/host/Linux-SGX/tools/common/util.h line 104 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
const char* maskedI think it should be rather named
mask.
If you feel like that's better, OK. Renamed.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 322 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Please use a more descriptive name (doesn't have to me long, just not a single random letter).
Not new code, but I agree, a more descriptive name would be better. Cleaned it up a bit.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @NieDzejkob)
Pal/src/host/Linux-SGX/tools/common/util.h line 104 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
If you feel like that's better, OK. Renamed.
Yeah, I prefer it because variable names usually describe "what is this variable", and "masked" sounds weird in this role.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 322 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Not new code, but I agree, a more descriptive name would be better. Cleaned it up a bit.
Oh, right. But now the function in longer so this name is more annoying ;)
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 292 at r3 (raw file):
} static bool truthy(const char *s) {
Suggestion:
const char* sPal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 323 at r3 (raw file):
/* successfully retrieved the secret: is it a key for encrypted files? */ const char *key_name = getenv(SECRET_PROVISION_SET_KEY);
Suggestion:
const char* key_namePal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 326 at r3 (raw file):
if (!key_name) { /* no key name specified - check old PF env var for compatibility */ const char *pf_key = getenv(SECRET_PROVISION_SET_PF_KEY);
Suggestion:
const char* pf_key
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @NieDzejkob)
Previously, mkow (Michał Kowalczyk) wrote…
Actually, maybe we should start doing it? It's technically separate from SGX PAL. Not blocking though.
It well may be that we will move RA-TLS and Secret Prov out of the core Gramine repo. Until we do so, let's keep the current style.
There was a problem hiding this comment.
Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @NieDzejkob)
Pal/src/host/Linux-SGX/tools/common/util.h line 104 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Yeah, I prefer it because variable names usually describe "what is this variable", and "masked" sounds weird in this role.
I thought of it as "masked content" or something like that.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 292 at r3 (raw file):
} static bool truthy(const char *s) {
Done.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 323 at r3 (raw file):
/* successfully retrieved the secret: is it a key for encrypted files? */ const char *key_name = getenv(SECRET_PROVISION_SET_KEY);
Done.
Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c line 326 at r3 (raw file):
if (!key_name) { /* no key name specified - check old PF env var for compatibility */ const char *pf_key = getenv(SECRET_PROVISION_SET_PF_KEY);
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @NieDzejkob)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
It well may be that we will move RA-TLS and Secret Prov out of the core Gramine repo. Until we do so, let's keep the current style.
sure
Pal/src/host/Linux-SGX/tools/common/util.h line 104 at r2 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I thought of it as "masked content" or something like that.
Ah. But without the noun it reads differently, at least for me ;)
Use the new /dev/attestation/keys/* interface when automatically provisioning keys for encrypted files. Also allow configuring the name of the key. Signed-off-by: Jakub Kądziołka <[email protected]>
mkow
force-pushed
the
NieDzejkob/secret-prov-pf
branch
from
6f797c2 to
fbfd07c
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)
Previously, mkow (Michał Kowalczyk) wrote…
sure
Done.
|
Jenkins, retest this please |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
This adapts the secret provisioning library and the example that uses it to make use of the new encrypted files APIs.
How to test this PR?
The
ra-tls-secret-provexample, which is run by CI, should almost fully suffice, though the warnings and errors printed in some conditions need to be checked manually.This change is