Support multiple keys in encrypted files #504
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 18 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: ITL) (waiting on @pwmarcz)
a discussion (no related file):
This PR also fixes #502 (so that the above update has correct error handling).
Could you split this out into a separate PR? We could merge it quickly, as we already agree on the solution and it's not too much code. And the feature implemented in the rest of this PR is non-trivial, so it may take a while to review.
There was a problem hiding this comment.
Reviewable status: 0 of 18 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: ITL) (waiting on @pwmarcz)
-- commits, line 26 at r1:
Actually, this commit can be also moved to a separate PR (where struct shim_mount_params doesn't have the field key).
This would be simpler to review. I'm not insisting though.
LibOS/shim/test/regression/tests.toml, line 53 at r1 (raw file):
"host_root_fs", "init_fail", "keys",
Forgot about musl-based tests?
There was a problem hiding this comment.
Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
This PR also fixes #502 (so that the above update has correct error handling).
Could you split this out into a separate PR? We could merge it quickly, as we already agree on the solution and it's not too much code. And the feature implemented in the rest of this PR is non-trivial, so it may take a while to review.
Done: #508
I'll remove this commit when the other one is merged.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, this commit can be also moved to a separate PR (where
struct shim_mount_paramsdoesn't have the fieldkey).This would be simpler to review. I'm not insisting though.
Done: #508
I'll remove this commit when the other one is merged.
LibOS/shim/test/regression/tests.toml, line 53 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Forgot about musl-based tests?
Yes, I did :) This is fixed now.
There was a problem hiding this comment.
Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)
Previously, pwmarcz (Paweł Marczewski) wrote…
Done: #508
I'll remove this commit when the other one is merged.
I don't see the change. And why do you move it to the already-existing PR #508? Note that I'm talking about a different commit than Michal meant.
There was a problem hiding this comment.
Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't see the change. And why do you move it to the already-existing PR #508? Note that I'm talking about a different commit than Michal meant.
Oh, sorry, that's a different one.
OK, I'll split this as well :)
There was a problem hiding this comment.
Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
Previously, pwmarcz (Paweł Marczewski) wrote…
Oh, sorry, that's a different one.
OK, I'll split this as well :)
Submitted as #510.
I'll rebase this PR after these two PRs are.
There was a problem hiding this comment.
Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
Previously, pwmarcz (Paweł Marczewski) wrote…
Submitted as #510.
I'll rebase this PR after these two PRs are.
*are merged.
There was a problem hiding this comment.
Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @pwmarcz)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Done: #508
I'll remove this commit when the other one is merged.
That's merged now, and I removed the commit from this PR.
Previously, pwmarcz (Paweł Marczewski) wrote…
*are merged.
That PR is merged now, and this commit only adds the key parameter now.
There was a problem hiding this comment.
Reviewed 5 of 11 files at r3.
Reviewable status: 5 of 19 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: ITL) (waiting on @pwmarcz)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
That's merged now, and I removed the commit from this PR.
Please also update the description of this PR.
LibOS/shim/test/regression/manifest.template, line 20 at r3 (raw file):
{ type = "tmpfs", path = "/mnt/tmpfs" }, { type = "encrypted", path = "/tmp/enc", uri = "file:tmp/enc", key = "custom" },
I'd rather use something like some_custom_name to clearly indicate to the users that these are arbitrary, not some magic names with special handling in Gramine.
There was a problem hiding this comment.
Reviewable status: 5 of 19 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Please also update the description of this PR.
Done (I removed the part about fixing #502, since the fix was merged separately).
LibOS/shim/test/regression/manifest.template, line 20 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd rather use something like
some_custom_nameto clearly indicate to the users that these are arbitrary, not some magic names with special handling in Gramine.
Renamed to my_custom_key, I hope that looks sufficiently non-built-in.
There was a problem hiding this comment.
Reviewed 5 of 18 files at r1, 1 of 1 files at r2, 6 of 11 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
LibOS/shim/include/shim_fs_encrypted.h, line 67 at r4 (raw file):
* \brief Retrieve a key. * * Returns a key with a given name, or NULL if it has not been created yet. Note that the even if
Note that the even -> Note that even
LibOS/shim/include/shim_fs_encrypted.h, line 68 at r4 (raw file):
* `struct shim_encrypted_files_key`). * * This does not pass ownership of `*out_key`: the key objects are still managed by this module.
Isn't this comment important to keep?
LibOS/shim/src/fs/shim_fs_encrypted.c, line 324 at r4 (raw file):
int ret = callback(key, arg); if (ret < 0) return ret;
You should unlock first.
LibOS/shim/src/fs/shim_fs_encrypted.c, line 667 at r4 (raw file):
struct shim_encrypted_files_key* key; int ret = get_or_create_encrypted_files_key(migrated_key->name, &key);
So instead of using the inside-checkpoint-blob migrated_key object, you're creating a new key object, add it to the list anew, and copy the contents from migrated_key. This is on purpose, right?
LibOS/shim/src/fs/dev/attestation.c, line 16 at r4 (raw file):
* correctly synchronize concurrent accesses to the pseudo-files. We expect attestation flows to * be generally single-threaded and therefore do not introduce synchronization here. */
Maybe add some TODO to remove:
g_pf_key_hexpfkey_loadpfkey_save
LibOS/shim/src/fs/dev/attestation.c, line 299 at r4 (raw file):
static int key_list_names_callback(struct shim_encrypted_files_key* key, void* arg) { struct key_list_names_data* data = arg; return data->callback(key->name, data->arg);
Why so complicated? You now have a callback that calls another callback. Can't you just specify list_encrypted_files_keys(readdir_callback_t callback, ...) as a function signature and use a single indirection?
LibOS/shim/src/fs/dev/attestation.c, line 339 at r4 (raw file):
if (size != KEY_HEX_LEN) { log_debug("/dev/attestation/protected_files_key: invalid length");
That's actually /dev/attestation/keys/
LibOS/shim/src/fs/dev/attestation.c, line 360 at r4 (raw file):
if (!strcmp(g_pal_public_state->host_type, "Linux-SGX")) { struct pseudo_node* user_report_data = pseudo_add_str(attestation, "user_report_data", NULL);
Can we add some log_debug() message here? Would be nice to tell something like
host is Linux-SGX, adding SGX-specific attestation files: report, quote, etc.
LibOS/shim/test/regression/keys.c, line 18 at r4 (raw file):
#include "rw_file.h" #define KEY_PATH "/dev/attestation/keys/my_custom_key"
Can you add a comment to look for this key and its original value in manifest.template?
LibOS/shim/test/regression/keys.c, line 36 at r4 (raw file):
if (strcmp(key, expected_key)) { errx(1, "%s: wrong key: expected %s, got %s", desc, key, expected_key);
Reorder key and expected_key arguments -- your string wants them in different order
LibOS/shim/test/regression/manifest.template, line 47 at r4 (raw file):
] fs.insecure__keys.my_custom_key = "00112233445566778899aabbccddeeff"
What about we also specify the default key, and augment the test above to check it as well?
This way, we'll additionally test two things:
- The linked list of keys
- The checkpoint-restore of even those keys that didn't have the associated file (
/dev/attestation/keys/default) opened/written to.
LibOS/shim/test/regression/manifest.template, line 47 at r4 (raw file):
] fs.insecure__keys.my_custom_key = "00112233445566778899aabbccddeeff"
Maybe also add a comment to see the keys.c test?
There was a problem hiding this comment.
Reviewable status: 14 of 19 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
LibOS/shim/include/shim_fs_encrypted.h, line 67 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Note that the even->Note that even
Done.
LibOS/shim/include/shim_fs_encrypted.h, line 68 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Isn't this comment important to keep?
Right. Restored.
LibOS/shim/src/fs/shim_fs_encrypted.c, line 324 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You should unlock first.
Ouch. Fixed.
LibOS/shim/src/fs/shim_fs_encrypted.c, line 667 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So instead of using the inside-checkpoint-blob
migrated_keyobject, you're creating a newkeyobject, add it to the list anew, and copy the contents frommigrated_key. This is on purpose, right?
Yes, this is on purpose. I think it deserves a comment - I added one.
LibOS/shim/src/fs/dev/attestation.c, line 16 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe add some TODO to remove:
g_pf_key_hexpfkey_loadpfkey_save
Isn't it too early for that? The current system ("protected files") is not even deprecated yet.
I agree that these need to be removed eventually, but first we'll need to switch all these APIs to use encrypted keys (e.g. pfkey_load will call key_load for the "default" key... or even the file could become a symlink), and document type = "encrypted" to be the right way of using the feature.
Then, I think, will be the right time to start the deprecation period, and add TODOs to delete all of these compatibility shims.
LibOS/shim/src/fs/dev/attestation.c, line 299 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why so complicated? You now have a callback that calls another callback. Can't you just specify
list_encrypted_files_keys(readdir_callback_t callback, ...)as a function signature and use a single indirection?
I regret the boilerplate, but I'd rather keep the module (shim_fs_encrypted.[ch]) independent from the rest of LibOS, and using the readdir_callback_t type breaks that.
I guess I could say that list_encrypted_files_keys actually works on names, and declare it to take a callback of form int (*callback)(const char* name, void* arg), which happens to be the same type as readdir_callback_t. The types would be even enforced by compiler.
But still, I don't think this would be a good situation, the coupling is essentially still there: if we ever change how readdir works, now we also have to either reach into shim_fs_encrypted.[ch] and change it as well, or rewrite the code here.
LibOS/shim/src/fs/dev/attestation.c, line 339 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
That's actually
/dev/attestation/keys/
Right. Fixed.
LibOS/shim/src/fs/dev/attestation.c, line 360 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can we add some
log_debug()message here? Would be nice to tell something likehost is Linux-SGX, adding SGX-specific attestation files: report, quote, etc.
Done.
LibOS/shim/test/regression/keys.c, line 18 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you add a comment to look for this key and its original value in
manifest.template?
Done.
LibOS/shim/test/regression/keys.c, line 36 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Reorder
keyandexpected_keyarguments -- your string wants them in different order
Right. Fixed.
LibOS/shim/test/regression/manifest.template, line 47 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What about we also specify the
defaultkey, and augment the test above to check it as well?This way, we'll additionally test two things:
- The linked list of keys
- The checkpoint-restore of even those keys that didn't have the associated file (
/dev/attestation/keys/default) opened/written to.
OK. Updated.
LibOS/shim/test/regression/manifest.template, line 47 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe also add a comment to
see the keys.c test?
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
LibOS/shim/src/fs/dev/attestation.c, line 16 at r4 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Isn't it too early for that? The current system ("protected files") is not even deprecated yet.
I agree that these need to be removed eventually, but first we'll need to switch all these APIs to use encrypted keys (e.g.
pfkey_loadwill callkey_loadfor the"default"key... or even the file could become a symlink), and documenttype = "encrypted"to be the right way of using the feature.Then, I think, will be the right time to start the deprecation period, and add TODOs to delete all of these compatibility shims.
OK, hope we won't forget to remove all these things.
LibOS/shim/src/fs/dev/attestation.c, line 299 at r4 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
I regret the boilerplate, but I'd rather keep the module (
shim_fs_encrypted.[ch]) independent from the rest of LibOS, and using thereaddir_callback_ttype breaks that.I guess I could say that
list_encrypted_files_keysactually works on names, and declare it to take a callback of formint (*callback)(const char* name, void* arg), which happens to be the same type asreaddir_callback_t. The types would be even enforced by compiler.But still, I don't think this would be a good situation, the coupling is essentially still there: if we ever change how
readdirworks, now we also have to either reach intoshim_fs_encrypted.[ch]and change it as well, or rewrite the code here.
Ok, I see, you want to keep shim_fs_encrypted.h completely separated.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
a discussion (no related file):
TODO: as discussed in #520, I'll convert /dev/attestation/keys/* files to use binary data instead of hex.
There was a problem hiding this comment.
Reviewable status: 13 of 19 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: ITL), "squash! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
TODO: as discussed in #520, I'll convert
/dev/attestation/keys/*files to use binary data instead of hex.
Done.
There was a problem hiding this comment.
Reviewed 5 of 6 files at r6, all commit messages.
Reviewable status: 18 of 19 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: ITL), "squash! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
LibOS/shim/src/fs/shim_fs_encrypted.c, line 204 at r6 (raw file):
log_warning("%s: wrong key length (%zu instead of %zu)", __func__, len, (size_t)(PF_KEY_SIZE * 2)); return -1;
Why not just -EINVAL?
LibOS/shim/src/fs/shim_fs_encrypted.c, line 213 at r6 (raw file):
if (hi < 0 || lo < 0) { log_warning("%s: unexpected character encountered", __func__); return -1;
Why not just -EINVAL?
LibOS/shim/src/fs/shim_fs_encrypted.c, line 285 at r6 (raw file):
if (ret < 0) { log_error("Cannot parse key 'fs.insecure__keys.%s' as a hex key", name); ret = -EINVAL;
If you change the error code in the function, you won't need this
LibOS/shim/test/regression/keys.c, line 39 at r6 (raw file):
0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77}; static void format_key(char buf[static KEY_STR_SIZE], const pf_key_t* key) {
What does this mean: char buf[static KEY_STR_SIZE]? The first time I see this construct.
LibOS/shim/test/regression/keys.c, line 82 at r6 (raw file):
expect_key("before writing key", CUSTOM_KEY_PATH, &custom_key); ssize_t n = posix_file_write(CUSTOM_KEY_PATH, (char*)&new_custom_key,
Can you add a comment that this key-write is invalid because the size is too small?
LibOS/shim/test/regression/keys.c, line 85 at r6 (raw file):
sizeof(new_custom_key) - 1); if (n >= 0 || (n < 0 && errno != EACCES)) err(1, "writing invalid key: expected EACCES");
Can you add an empty line after this?
There was a problem hiding this comment.
Reviewed 3 of 18 files at r1, 1 of 1 files at r2, 4 of 11 files at r3, 1 of 5 files at r5, 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)
LibOS/shim/include/shim_fs.h, line 40 at r6 (raw file):
/* Key name (used by `chroot_encrypted` filesystem), or NULL if not applicable */ const char* key;
Maybe key_name instead? Would be less ambiguous and still not overly long.
LibOS/shim/include/shim_fs_encrypted.h, line 93 at r6 (raw file):
* * \param key The key to read. * \param[out] pf_key On success, will be set to new value.
new? copy-paste error, I guess?
Code quote:
new valueLibOS/shim/src/fs/shim_fs_encrypted.c, line 673 at r6 (raw file):
lock(&g_keys_lock); key->is_set = migrated_key->is_set; memcpy(&key->pf_key, &migrated_key->pf_key, sizeof(migrated_key->pf_key));
I assume we can't use update_encrypted_files_key() here because the key may not be set, right?
LibOS/shim/test/regression/keys.c, line 39 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What does this mean:
char buf[static KEY_STR_SIZE]? The first time I see this construct.
In C and C++ you can basically put static in any place in the code and it's a valid construct, although it has a different meaning in each of them.
https://hamberg.no/erlend/posts/2013-02-18-static-array-indices.html
LibOS/shim/test/regression/keys.c, line 40 at r6 (raw file):
01234567890
🤔
LibOS/shim/test/regression/keys.c, line 53 at r6 (raw file):
pf_key_t key; ssize_t n = posix_file_read(path, (char*)key, sizeof(key));
Suggestion:
(char*)&keyLibOS/shim/test/regression/keys.c, line 56 at r6 (raw file):
if (n < 0) err(1, "%s: error reading %s", desc, path); if (n < (ssize_t)sizeof(key))
Suggestion:
(size_t)n < sizeof(key)There was a problem hiding this comment.
Reviewable status: 14 of 19 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @pwmarcz)
LibOS/shim/include/shim_fs.h, line 40 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Maybe
key_nameinstead? Would be less ambiguous and still not overly long.
Makes sense. Renamed.
We still want the manifest syntax to be { ..., key = "foo" }, not { ..., key_name = "foo" }, right?
LibOS/shim/include/shim_fs_encrypted.h, line 93 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
new? copy-paste error, I guess?
Right, it's current value. Fixed.
LibOS/shim/src/fs/shim_fs_encrypted.c, line 204 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not just
-EINVAL?
This is consistent with other "parse" type functions (there's one in pseudo, and some in api.h, like parse_size_str or str_to_ulong) - they don't really return error codes because there's only one type of error possible, so I think it's better to just treat it as success/failure.
LibOS/shim/src/fs/shim_fs_encrypted.c, line 673 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I assume we can't use
update_encrypted_files_key()here because the key may not be set, right?
Yeah... I could do if (is_set) update_encrypted_files_key(), and it would probably work (if the key is not set in parent, that means the child won't have initialized it either), but that's unobvious and brittle. I feel better about copying data directly.
LibOS/shim/test/regression/keys.c, line 39 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
In C and C++ you can basically put
staticin any place in the code and it's a valid construct, although it has a different meaning in each of them.
https://hamberg.no/erlend/posts/2013-02-18-static-array-indices.html
As I understand, this means that buf is a valid pointer to at least KEY_STR_SIZE elements.
Sadly, this doesn't make GCC warn on passing a smaller array or NULL (although when caller is in the same file, it will warn anyway). So basically it's for documentation purposes.
LibOS/shim/test/regression/keys.c, line 40 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
01234567890
🤔
Haha :) Good thing it's only a test. Fixed.
LibOS/shim/test/regression/keys.c, line 53 at r6 (raw file):
pf_key_t key; ssize_t n = posix_file_read(path, (char*)key, sizeof(key));
Done.
LibOS/shim/test/regression/keys.c, line 56 at r6 (raw file):
if (n < 0) err(1, "%s: error reading %s", desc, path); if (n < (ssize_t)sizeof(key))
I agree. Done.
LibOS/shim/test/regression/keys.c, line 82 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you add a comment that this key-write is invalid because the size is too small?
Done.
LibOS/shim/test/regression/keys.c, line 85 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you add an empty line after this?
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @pwmarcz)
LibOS/shim/include/shim_fs.h, line 40 at r6 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Makes sense. Renamed.
We still want the manifest syntax to be
{ ..., key = "foo" }, not{ ..., key_name = "foo" }, right?
Good question, I think I actually prefer the latter, but not super strongly.
@boryspoplawski, @dimakuv ^
LibOS/shim/test/regression/keys.c, line 40 at r6 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Haha :) Good thing it's only a test. Fixed.
Yeah, but people sometimes copy code from tests to the main codebase :)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)
LibOS/shim/include/shim_fs.h, line 40 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Good question, I think I actually prefer the latter, but not super strongly.
@boryspoplawski, @dimakuv ^
I did not review the whole PR (should I?) so I need more context - that's an identifier for a key, so that we can pick appropriate key from specific /dev entry, right? If that's correct, then I would prefer key_name or key_id - just key sounds like an inline key or a path to a file containing it.
There was a problem hiding this comment.
Reviewed 1 of 6 files at r6, 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @mkow)
LibOS/shim/include/shim_fs.h, line 40 at r6 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I did not review the whole PR (should I?) so I need more context - that's an identifier for a key, so that we can pick appropriate key from specific
/deventry, right? If that's correct, then I would preferkey_nameorkey_id- justkeysounds like an inline key or a path to a file containing it.
I agree with Michal and Borys that key_name is less ambigious. So I vote for this rename in the manifest syntax as well. (I don't like key_id, because IDs for me are some random numbers, but here we're talking about human-readable human-defined names.)
There was a problem hiding this comment.
Reviewable status: 12 of 19 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
LibOS/shim/include/shim_fs.h, line 40 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree with Michal and Borys that
key_nameis less ambigious. So I vote for this rename in the manifest syntax as well. (I don't likekey_id, because IDs for me are some random numbers, but here we're talking about human-readable human-defined names.)
OK. I changed the manifest syntax to key_name as well.
There was a problem hiding this comment.
Reviewed 2 of 7 files at r8, 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 2 of 7 files at r8, 1 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
Signed-off-by: Paweł Marczewski <pawel@invisiblethingslab.com>
The keys can be modified through the special files at `/dev/attestation/keys/<key_name>`. Unlike `/dev/attestation/protected_files_key`, the new files use raw binary, not hex strings. Signed-off-by: Paweł Marczewski <pawel@invisiblethingslab.com>
There was a problem hiding this comment.
Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)
There was a problem hiding this comment.
Reviewed 2 of 4 files at r10, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
This allows mounting encrypted files with
key = "..."parameter. In addition, the keys can be now updated by application, by writing to/dev/attestation/keys/<name>.How to test this PR?
The LibOS regression tests explicitly use
key = "custom"now. There is also a test that checks if we can set a key, if errors are handled, and if the update persists after a fork.This change is