[common] Refactor Protected Files slightly #1841
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 5 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: Intel)
a discussion (no related file):
FYI: I will submit a new ReadTheDocs document with description of Protected/Encrypted Files. The names in that document will correspond to the names created in this PR.
common/src/protected_files/protected_files_internal.h line 16 at r1 (raw file):
#include "protected_files_format.h" struct pf_context {
For reviewers' convenience, renames:
file_metadata->metadata_nodeencrypted_part_plain->metadata_decrypted_headerroot_mht->root_mht_nodefile->host_file_handleuser_kdk_key->kdk
tools/sgx/pf_tamper/pf_tamper.c line 160 at r1 (raw file):
truncate_file("trunc_meta_plain_3", offsetof(metadata_plaintext_header_t, minor_version)); truncate_file("trunc_meta_plain_4", offsetof(metadata_plaintext_header_t, metadata_key_salt)); truncate_file("trunc_meta_plain_5", FIELD_TRUNCATED(metadata_plaintext_header_t, metadata_key_salt));
FYI: I decided not to care about 100-char limit in this file -- maybe it's time to just delete this file, as it is broken for several years already...
There was a problem hiding this comment.
I like the house-cleaning of removing unused stuff and i also like the renaming which makes names clearer. Only "complaint" i have is that pf_tamper.c is imho still used and while functionally still correct according to my tests probably will have to be properly formatted? (100 char line limits)?
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all 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) (waiting on @dimakuv)
common/src/protected_files/protected_files_internal.h line 16 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
For reviewers' convenience, renames:
file_metadata->metadata_nodeencrypted_part_plain->metadata_decrypted_headerroot_mht->root_mht_nodefile->host_file_handleuser_kdk_key->kdk
nonce/id -> salt seems another criticial (but meaningful) rename?
tools/sgx/pf_tamper/pf_tamper.c line 160 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: I decided not to care about 100-char limit in this file -- maybe it's time to just delete this file, as it is broken for several years already...
Sorry for my ignorance but looking at code i thought pf_tamper is still used libos/test/fs/test_enc.py and seems to work in that case, so at least only partially broken? In any case, deleting the file would break these tests? In any case, your changes seem to work as (cd libos/test/fs; gramine-test pytest -v) passes properly for me.
This commit refactors PF code without changing functionality: - Many renames of data structs, variable names, func names - More readable comments (and fixing incorrect comments) - Removed unused `pf_get_handle()` - Removed unused `g_pf_mrenclave_key`, `g_pf_wrap_key`, etc. - Removed all mentions of `list`/`LIST` operations (were unused) This commit contains changes (renames) in `pf_tamper.c`, but this file and corresponding tool are not tested and most probably broken. We did the changes purely to survive compilation. Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
dimakuv
force-pushed
the
dimakuv/protected-files-cleanup
branch
from
ae14b3d
to
c17f4ce
Compare
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @g2flyer)
common/src/protected_files/protected_files_internal.h line 16 at r1 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
nonce/id -> salt seems another criticial (but meaningful) rename?
Yes, sure. This question is not actionable, right? I don't see anywhere in the code/commit message where I can add this info on nonce -> salt.
tools/sgx/pf_tamper/pf_tamper.c line 160 at r1 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
Sorry for my ignorance but looking at code i thought
pf_tamperis still usedlibos/test/fs/test_enc.pyand seems to work in that case, so at least only partially broken? In any case, deleting the file would break these tests? In any case, your changes seem to work as(cd libos/test/fs; gramine-test pytest -v)passes properly for me.
Done, you're right, my bad. It is still used, see test_enc.py::TC_50_EncryptedFiles::test_500_invalid.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 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: Intel) (waiting on @dimakuv)
common/src/protected_files/protected_files_internal.h line 16 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, sure. This question is not actionable, right? I don't see anywhere in the code/commit message where I can add this info on
nonce -> salt.
Nope, put it only there for other reviewers. Sorry, still learning your conventions in reviews :-)
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
There was a problem hiding this comment.
Reviewable status: 1 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)
a discussion (no related file):
FYI: Based on review of #1845, I changed mac -> tag everywhere.
-- commits line 13 at r3:
TODO: Change this incorrect sentence. The tool is used. Also, we modified slightly the LibOS too.
There was a problem hiding this comment.
Reviewed 6 of 7 files at r3, all commit messages.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: Based on review of #1845, I changed
mac->tageverywhere.
I see why this other comment lead you to a change here for consistency. In the other review, though, i was mainly objecting to the use of _g_mac which seemed technically incorrect (whereas mac as more general and a bit vaguer term still could be considered still somewhat ok here). I fear thattag with no attribution to authentication seems not to capture the meaning in the name alone and folks would have to look at comment in structure definition to know for sure . I probably would use auth_tag instead of tag or just left mac
This reverts commit 6c008a3.
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
There was a problem hiding this comment.
Reviewable status: 1 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)
a discussion (no related file):
Previously, g2flyer (Michael Steiner) wrote…
I see why this other comment lead you to a change here for consistency. In the other review, though, i was mainly objecting to the use of _g_mac which seemed technically incorrect (whereas mac as more general and a bit vaguer term still could be considered still somewhat ok here). I fear that
tagwith no attribution to authentication seems not to capture the meaning in the name alone and folks would have to look at comment in structure definition to know for sure . I probably would useauth_taginstead oftagor just leftmac
Done, reverted the previous change and renamed now from gmac -> mac.
There was a problem hiding this comment.
Looks so good i couldn't find anything else to comment on :-)
Reviewed 7 of 7 files at r4, 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: Intel), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Could you split this into smaller PRs? There are a lot of independent changes here, despite that "slightly" in the name ;) (the diff has ~600 lines)
There was a problem hiding this comment.
Reviewable status: all 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 @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you split this into smaller PRs? There are a lot of independent changes here, despite that "slightly" in the name ;) (the diff has ~600 lines)
Hm, the diff itself is pretty simple. Most of the lines are trivial renames of variables.
But yes, I can split this PR into several ones. What about this split:
- Remove unused functions and variables
- Modify some weird
while()loops into more readablefor()loops - Remove redundant/incorrect comments
- Move structs around
- Renames of data structs, variable names, func names (this PR I'll do only when PRs before are merged, otherwise merge conflicts will be too painful for me)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, the diff itself is pretty simple. Most of the lines are trivial renames of variables.
But yes, I can split this PR into several ones. What about this split:
- Remove unused functions and variables
- Modify some weird
while()loops into more readablefor()loops- Remove redundant/incorrect comments
- Move structs around
- Renames of data structs, variable names, func names (this PR I'll do only when PRs before are merged, otherwise merge conflicts will be too painful for me)
Sounds good. The split should make the merging faster, because I guess there won't be any discussion for most of these parts, but there can be a few for the comment rewrite and variable renaming.
There was a problem hiding this comment.
Reviewable status: all 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 @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Sounds good. The split should make the merging faster, because I guess there won't be any discussion for most of these parts, but there can be a few for the comment rewrite and variable renaming.
- [common] Refactor Protected Files, part 1 #1866
- [common] Refactor Protected Files, part 2 #1867
- [common] Refactor Protected Files, part 3 #1868
- [common] Refactor Protected Files, part 4 #1869
This PR was split in 4 above PRs + there will be one more PR after the above ones are merged (that's to not have too many merge conflicts).
Description of the changes
This PR refactors PF code without changing functionality:
pf_get_handle()g_pf_mrenclave_key,g_pf_wrap_key, etc.list/LISToperations (were unused)This PR contains changes (renames) in
pf_tamper.c, but this file and corresponding tool are not tested and most probably broken. We did the changes purely to survive compilation.How to test this PR?
No functional changes, CI is enough.
This change is