[Docs] Add chapter on encrypted files implementation #1845
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 11 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)
a discussion (no related file):
Blocking on #1841
dimakuv
force-pushed
the
dimakuv/docs-encrypted-files
branch
2 times, most recently
from
dc24f20
to
4328441
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 11 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)
Documentation/devel/encfiles.rst line 289 at r2 (raw file):
derivation, with input materials being the KDK and the salt (plus the hard-coded label ``SGX-PROTECTED-FS-METADATA-KEY`` and the hard-coded integers ``1`` and ``128``).
I'm not sure these hard-coded label and integers are needed in this description. Moreover, I don't know why they are important at all. TODO: Ask crypto experts.
There was a problem hiding this comment.
This is a very helpful PR for anybody who wants to understand how protected files work and how it's implemented! Find though a few suggestion for potential improvements.
Reviewed 10 of 11 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 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)
Documentation/devel/encfiles.rst line 19 at r3 (raw file):
the application must be put under the "encrypted" FS mount in the Gramine manifest. New files created in the "encrypted" FS mount are automatically treated as encrypted. The encryption format used for encrypted files is borrowed
suggest s/encrypted/encrypted and integrity-protected/ and probably would drop the 'encryption' before format or call it file-format
Documentation/devel/encfiles.rst line 26 at r3 (raw file):
Each encrypted file is encrypted separately, i.e. Gramine employs file-level encryption and not block-level encryption. Each "encrypted" FS mount can have a separate encryption key. By putting each encrypted file in its own FS mount, it
we probably should call this key key derivation key as the actual file is not encrypted with that key but a key derived from that KDK. In fact, we later call it KDK so we best already name it here the same and maybe even explicitly that files are encrypted with derived key(s).
BTW: While strictly true, i think the subsequent "By putting each ..." is also a bit misleading and in any case personally i wouldn't explicitly put that in the text in such prominent form: this should be an uncommon (and undesirable) case and if somebody really needs it they will figure it out ...
Documentation/devel/encfiles.rst line 45 at r3 (raw file):
- **Confidentiality of user data**: all user data is encrypted and then written to untrusted host storage; this prevents user data leakage. - **Integrity of user data**: all user data is read from disk and then decrypted
"decrypted with GMAC" is an oxymoron as GMAC wouldn't involve encryption and i think strictly speaking, the code also doesn't use GMAC (contrary to what some of the variables seem to indicate) nor even AAD from AES-GCM. So i would talk about AES-GCM as the overall scheme and when talking about the "MAC" use the standard AES-GCM term of "Authentication Tag" (i.e., s/GMAC/Authentication Tag/g)
Documentation/devel/encfiles.rst line 58 at r3 (raw file):
Gramine does not guarantee the freshness of user data in the file after this file was closed. Note that while the file is opened, the rollback/replay attack is prevented (by comparing the root MHT hashes).
"(by comparing the root MHT hashes)" -> "(by keeping the root of a Merkle-tree over the file always in (trusted) memory and checking the consistency during access, see more details below)" or alike might be more clear? At least term "merkle-tree" would seem useful as crucially keeping the root always in memory?
Documentation/devel/encfiles.rst line 59 at r3 (raw file):
file was closed. Note that while the file is opened, the rollback/replay attack is prevented (by comparing the root MHT hashes). - **Side-channel attacks**. Some seemingly-insignificant information, such as
I would drop the "Some seemingly-insignificant.
Documentation/devel/encfiles.rst line 62 at r3 (raw file):
file name, file size, access time, access patterns (e.g., which blocks are read/written), etc. is not protected. This information could be used by sophisticated attackers to gain sensitive information.
i would drop 'sophisticated'; this information is trivial to access and exploit if really sensitive?
BTW: this also reminds me that currently file meta data such as access time, ownership, permission bits, etc are also not integrity protected. Maybe also worth mentioning somewhere?
Documentation/devel/encfiles.rst line 98 at r3 (raw file):
- Reusability -- the encrypted-files code can be used as-is in stand-alone tools like :program:`gramine-sgx-pf-crypt`. - Crypto reviews -- the encrypted-files code contains only the crypto
I guess you wanted to say that the encrypted files code is the only one (directly) using crypto? Currently it sounds like it does only crypto but clearly it does more (in fact, it doesn't even implement the crypto itself, just calling it?
Documentation/devel/encfiles.rst line 101 at r3 (raw file):
algorithms, which facilitates crypto/security review efforts. The application code is *not* aware of encrypted files. Applications treat
Maybe worth, though, describe somewhere what happens when we detect tampering/integrity-inconsistencies. Looking at code currently i think currently we set file status to PF_STATUS_CORRUPTED and return an "standard" error but otherwise continue as-is? Makes me wonder whether maybe there should be an option to abort in such cases? (not that this would be something for the PR, though :-))
Documentation/devel/encfiles.rst line 118 at r3 (raw file):
``libos_encrypted_file`` data structure is hosted in the glue code, and the ``pf_context`` data structure is hosted in the generic encrypted-files code. The encryption key is installed through Gramine interfaces into the
s/encryption key/key derivation key/ (or just KDK?)
Documentation/devel/encfiles.rst line 225 at r3 (raw file):
``root_mht_node`` field. Also, the root MHT node's encryption key and GMAC are stored directly in the encrypted header of the metadata node. The root MHT node starts to be used when the plaintext file size exceeds 3KB.
maybe also worth mentioning that other nodes will be fetched on demand, cached and potentially evicted, the root MHT node will be always in memory for the lifetime of the file handle? (actually, notice that you mention that latter when discussing the LRU cache. So i guess it's fine to just mention it there ...)
Documentation/devel/encfiles.rst line 480 at r3 (raw file):
into the cache. - There is *no* multiprocess support for encrypted files. This means that if the
There is limited support for multi-process. The next sentence seems to allude to that though but arguably, contrary to the first sentence which generalizes on the safe side, seems a bit to to cover only a subset of problematic cases: the sentence can be read is if the issue is only if the open happens simultaneous but not if, e.g., opens are inherited from fork or alike? Maybe better to talk just about concurrent access of a file? (more precisely it would be concurrent access where one is writing but i guess that's a bit too much details and probably not the make or break of many use-case being supported or not.
Documentation/devel/encfiles.rst line 492 at r3 (raw file):
- There is no key rotation scheme. The application must perform key rotation of the KDK by itself (by overwriting the ``/dev/attestation/keys/`` pseudo-files). Some support for key rotation may appear in future releases of
Not sure whether worth pointing out but given that mostly we have one-time use keys, the issue is primarily the KDF which produces much less ciphertext than if it would be used to directly encrypt data, so the usual NIST limits would be reached much less quickly. If there would be lots of files, what could help in making rotation less critical is to add another level of indirection to derive from the KDK first a file-specific KDK?
There was a problem hiding this comment.
Reviewable status: 1 of 11 files reviewed, 14 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 @g2flyer)
Documentation/devel/encfiles.rst line 289 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'm not sure these hard-coded label and integers are needed in this description. Moreover, I don't know why they are important at all. TODO: Ask crypto experts.
@g2flyer Could I ask you with these labels and integers are important for deriving the keys?
Documentation/devel/encfiles.rst line 19 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
suggest s/encrypted/encrypted and integrity-protected/ and probably would drop the 'encryption' before format or call it file-format
Done.
Documentation/devel/encfiles.rst line 26 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
we probably should call this key
key derivation keyas the actual file is not encrypted with that key but a key derived from that KDK. In fact, we later call it KDK so we best already name it here the same and maybe even explicitly that files are encrypted with derived key(s).
BTW: While strictly true, i think the subsequent "By putting each ..." is also a bit misleading and in any case personally i wouldn't explicitly put that in the text in such prominent form: this should be an uncommon (and undesirable) case and if somebody really needs it they will figure it out ...
Done. 2x
Documentation/devel/encfiles.rst line 45 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
"decrypted with GMAC" is an oxymoron as GMAC wouldn't involve encryption and i think strictly speaking, the code also doesn't use GMAC (contrary to what some of the variables seem to indicate) nor even AAD from AES-GCM. So i would talk about AES-GCM as the overall scheme and when talking about the "MAC" use the standard AES-GCM term of "Authentication Tag" (i.e., s/GMAC/Authentication Tag/g)
Done. Using tag everywhere.
Documentation/devel/encfiles.rst line 58 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
"(by comparing the root MHT hashes)" -> "(by keeping the root of a Merkle-tree over the file always in (trusted) memory and checking the consistency during access, see more details below)" or alike might be more clear? At least term "merkle-tree" would seem useful as crucially keeping the root always in memory?
Done.
Documentation/devel/encfiles.rst line 59 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
I would drop the "Some seemingly-insignificant.
Done.
Documentation/devel/encfiles.rst line 62 at r3 (raw file):
Done.
this also reminds me that currently file meta data such as access time, ownership, permission bits, etc are also not integrity protected. Maybe also worth mentioning somewhere?
These file meta data are unused by Gramine. In particular:
- Access/modify/change timestamps are always zeroed-out in Gramine.
- Ownership is always the current user; ownership info from the host is ignored.
- Permission bits is always the current mask; permission info from the host is ignored.
We mention access/modify/change timestamps here: https://gramine.readthedocs.io/en/stable/devel/features.html#file-systems. But I agree that we don't explicitly mention about e.g. ownership (user/group) and permission peculiarities. Do you think I should add there the list that I have above?
Documentation/devel/encfiles.rst line 98 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
I guess you wanted to say that the encrypted files code is the only one (directly) using crypto? Currently it sounds like it does only crypto but clearly it does more (in fact, it doesn't even implement the crypto itself, just calling it?
Done.
Documentation/devel/encfiles.rst line 101 at r3 (raw file):
Done.
Makes me wonder whether maybe there should be an option to abort in such cases?
Do you think the application may be tricked into doing something else if the file was corrupted? I would think that it is the app's responsibility to decide what to do with a file that is not accessible. But if you have a use case, then yes, we could add such a knob to just fail fast the whole process.
Documentation/devel/encfiles.rst line 118 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
s/encryption key/key derivation key/ (or just KDK?)
Done.
Documentation/devel/encfiles.rst line 225 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
maybe also worth mentioning that other nodes will be fetched on demand, cached and potentially evicted, the root MHT node will be always in memory for the lifetime of the file handle? (actually, notice that you mention that latter when discussing the LRU cache. So i guess it's fine to just mention it there ...)
Done.
Documentation/devel/encfiles.rst line 480 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
There is limited support for multi-process. The next sentence seems to allude to that though but arguably, contrary to the first sentence which generalizes on the safe side, seems a bit to to cover only a subset of problematic cases: the sentence can be read is if the issue is only if the open happens simultaneous but not if, e.g., opens are inherited from fork or alike? Maybe better to talk just about concurrent access of a file? (more precisely it would be concurrent access where one is writing but i guess that's a bit too much details and probably not the make or break of many use-case being supported or not.
Done.
Documentation/devel/encfiles.rst line 492 at r3 (raw file):
Done
If there would be lots of files, what could help in making rotation less critical is to add another level of indirection to derive from the KDK first a file-specific KDK?
I am no crypto expert. Intuitively it feels like bumping up the overall security. I don't know who can run such crypto analysis, and if it's needed at all.
There was a problem hiding this comment.
Looks good. See a few follow-up suggestion though.
Reviewed 9 of 10 files at r4, all commit messages.
Reviewable status: 10 of 11 files reviewed, 5 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)
Documentation/devel/encfiles.rst line 289 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@g2flyer Could I ask you with these labels and integers are important for deriving the keys?
i guess you refer to the buf.index=1 and buf.output_len=0x80 from ipf\_import\_metadata\_key. These are artifacts of the KDF construction of NIST-SP800-108: The index is used to handle the case where you need more key-material than a single invocation of a PRF (in this case aes-cmac) is required where each invocation increases the index. And the output length is to fix how many bits you really need (and so different lengths give completely different bitstreams for same label and you can't truncate the longer one to get the shorter one). As we are generating AES-128 keys, hence the 128.
So i would drop these constants altogether and probably rephrase the paragraph along the lines of
"Then in step 3, a NIST SP800-108 KDF (Key Derivation Function) is used to derive a sub-key, with input materials being the KDK, the hard-coded label SGX-PROTECTED-FS-METADATA-KEY and the salt as nonce." (Note in SP800-104 the place we put salt is called nonce, hence my reference to that.)
Documentation/devel/encfiles.rst line 62 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
this also reminds me that currently file meta data such as access time, ownership, permission bits, etc are also not integrity protected. Maybe also worth mentioning somewhere?
These file meta data are unused by Gramine. In particular:
- Access/modify/change timestamps are always zeroed-out in Gramine.
- Ownership is always the current user; ownership info from the host is ignored.
- Permission bits is always the current mask; permission info from the host is ignored.
We mention access/modify/change timestamps here: https://gramine.readthedocs.io/en/stable/devel/features.html#file-systems. But I agree that we don't explicitly mention about e.g. ownership (user/group) and permission peculiarities. Do you think I should add there the list that I have above?
Given that you mention this already in features (as it is more general than PF), why not just add here a link to that section along the lines of "See also [link] for additional discussion on file metadata" or alike?
Documentation/devel/encfiles.rst line 101 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
Makes me wonder whether maybe there should be an option to abort in such cases?
Do you think the application may be tricked into doing something else if the file was corrupted? I would think that it is the app's responsibility to decide what to do with a file that is not accessible. But if you have a use case, then yes, we could add such a knob to just fail fast the whole process.
Of course an application should always check return codes and alike. But in general applications are written under the assumption nothing malicious happen under the cover. E.g., an adversary could try to inject some faults and alike. That said, there probably more effective way to do such than causing random read or write failures so probably not worth. (And as far as i can tell, after an integrity failure, no read or write information would ever work again (i.e., there is no way to selectively suppress, say, some writes, or alike. Certainly looked like that in the code and i see now you also added a corresponding text below.)
Documentation/devel/encfiles.rst line 133 at r3 (raw file):
size and all GMACs are 16B in size. - AES-CMAC with AES-128-bit is used to derive keys from the user-supplied KDK.
Following up to comment elsewhere on constants 1 and 128, suggest changing here also to something like
"Sub-keys are derived from the user-supplied KDK using the NIST SP800-108 construction, with the required PRF instantiated by AES-128-CMAC.''
There was a problem hiding this comment.
Reviewable status: 10 of 11 files reviewed, 5 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)
Documentation/devel/encfiles.rst line 289 at r2 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
i guess you refer to the buf.index=1 and buf.output_len=0x80 from
ipf\_import\_metadata\_key. These are artifacts of the KDF construction of NIST-SP800-108: The index is used to handle the case where you need more key-material than a single invocation of a PRF (in this case aes-cmac) is required where each invocation increases the index. And the output length is to fix how many bits you really need (and so different lengths give completely different bitstreams for same label and you can't truncate the longer one to get the shorter one). As we are generating AES-128 keys, hence the 128.
So i would drop these constants altogether and probably rephrase the paragraph along the lines of
"Then in step 3, a NIST SP800-108 KDF (Key Derivation Function) is used to derive a sub-key, with input materials being the KDK, the hard-coded labelSGX-PROTECTED-FS-METADATA-KEYand the salt as nonce." (Note in SP800-104 the place we put salt is called nonce, hence my reference to that.)
minor correction to my suggestion: s/to derive a sub-key/to derive a AES-128 sub-key/ (hence also implicitly fixing the 1 & 128 constants)
There was a problem hiding this comment.
Reviewable status: 2 of 11 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 @g2flyer)
Documentation/devel/encfiles.rst line 289 at r2 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
minor correction to my suggestion: s/to derive a sub-key/to derive a AES-128 sub-key/ (hence also implicitly fixing the 1 & 128 constants)
Done. Thanks for all the info, fixed according to your suggestion!
Btw, the standard is pretty easy to read, included a link to it in the text (https://csrc.nist.gov/pubs/sp/800/108/r1/upd1/final)
Documentation/devel/encfiles.rst line 62 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
Given that you mention this already in features (as it is more general than PF), why not just add here a link to that section along the lines of "See also [link] for additional discussion on file metadata" or alike?
Done. (I cannot insert a more specific link to the File Systems chapter because I don't understand how to do such cross-linking between RestructedText and Markdown files.)
Documentation/devel/encfiles.rst line 101 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
Of course an application should always check return codes and alike. But in general applications are written under the assumption nothing malicious happen under the cover. E.g., an adversary could try to inject some faults and alike. That said, there probably more effective way to do such than causing random read or write failures so probably not worth. (And as far as i can tell, after an integrity failure, no read or write information would ever work again (i.e., there is no way to selectively suppress, say, some writes, or alike. Certainly looked like that in the code and i see now you also added a corresponding text below.)
Hm, ok, I won't create a GitHub issue for this, since (1) this behavior is already described in this doc, and (2) we both agree that it's probably not worth it. If anyone else comes with a legitimate use case, then we'll resurrect the discussion.
Documentation/devel/encfiles.rst line 133 at r3 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
Following up to comment elsewhere on constants 1 and 128, suggest changing here also to something like
"Sub-keys are derived from the user-supplied KDK using the NIST SP800-108 construction, with the required PRF instantiated by AES-128-CMAC.''
Done.
There was a problem hiding this comment.
Reviewable status: 2 of 11 files reviewed, 5 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 @g2flyer)
Documentation/devel/encfiles.rst line 8 at r5 (raw file):
This is a living document. The last major update happened in **April 2024** and closely corresponds to Gramine v1.6.
During rebase, change this to v1.7 (released in the meantime).
Also, probably it won't be April 2024.
There was a problem hiding this comment.
Reviewable status: 2 of 11 files reviewed, 5 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 @g2flyer)
Documentation/devel/encfiles.rst line 62 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. (I cannot insert a more specific link to the File Systems chapter because I don't understand how to do such cross-linking between RestructedText and Markdown files.)
Update: apparently I can't even insert an RST reference to the Markdown file... I circumvented this by specifying a normal HTTPS link.
There was a problem hiding this comment.
I ran out of more suggestions to give, i.e., i approve (but just in this text as i don't seem to have the rights to in reviewable to actually hit the approve button)
Reviewed 8 of 9 files at r5, 1 of 1 files at r6, 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), "fixup! " found in commit messages' one-liners
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
a discussion (no related file):
I will need to replace nonce with salt in all the text and diagrams. This rename was agreed here: https://reviewable.io/reviews/gramineproject/gramine/1869#-Ny7urK_6T31o1m9GkdO
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
dimakuv
force-pushed
the
dimakuv/docs-encrypted-files
branch
from
debc38c
to
9d94e30
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 11 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)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Blocking on #1841
Done. All relevant PRs were merged.
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I will need to replace
noncewithsaltin all the text and diagrams. This rename was agreed here: https://reviewable.io/reviews/gramineproject/gramine/1869#-Ny7urK_6T31o1m9GkdO
Done
There was a problem hiding this comment.
Reviewed 1 of 11 files at r1, 1 of 10 files at r4, 9 of 9 files at r7, 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: Intel) (waiting on @dimakuv)
Documentation/devel/encfiles.rst line 16 at r7 (raw file):
Gramine provides a feature of :ref:`encrypted-files`, which encrypts files and transparently decrypts them when the application reads or writes them. Integrity- or confidentiality-sensitive files (or whole directories) accessed by
-> and?
Code quote:
orDocumentation/devel/encfiles.rst line 144 at r7 (raw file):
<https://csrc.nist.gov/pubs/sp/800/108/r1/upd1/final>`__ construction, with the required PRF (Pseudorandom Function) instantiated by AES-128-CMAC.
what about the key for the data node (from rdrand)?
Documentation/devel/encfiles.rst line 179 at r7 (raw file):
between the SGX enclave and the host storage, - ``metadata_decrypted`` points to a data struct that contains the decrypted part of the metadata node's encrypted header,
-> encrypted part?
Code quote:
encrypted headerDocumentation/devel/encfiles.rst line 195 at r7 (raw file):
nonce for KDF (Key Derivation Function, explained later) and a MAC (cryptographic hash over the encrypted header). 2. The encrypted header, occupying bytes 58-3941. This header has two parts: the
Why we call it "encrypted header"? It's a bit confusing that it's noted in the diagram that the header of metadata node is in plaintext
Code quote:
The encrypted headerDocumentation/devel/encfiles.rst line 231 at r7 (raw file):
Note that there is a special MHT node -- the root MHT node. It has the same representation inside the SGX enclave and on host storage as all other MHT nodes, but it is directly linked from the main data struct ``pf_handle`` via the
-> pf_context?
Code quote:
pf_handleDocumentation/devel/encfiles.rst line 293 at r7 (raw file):
Upon file creation, Gramine sets up three data structures representing the file: the main ``pf_context`` struct that has the reference to the correspoding host
corresponding
Code quote:
correspodingDocumentation/devel/encfiles.rst line 332 at r7 (raw file):
Then the app wants to read the file data. This triggers the read flow depicted on the diagram above. The encrypted file is represented on the untrusted storage
-> in?
Code quote:
onDocumentation/devel/encfiles.rst line 403 at r7 (raw file):
is generated and is stored in the corresponding slot of the root MHT node (thus shaping a key + MAC pair for data node 1). Since the MHT node's contents will be encrypted, the MAC will not be leaked.
I think the MAC is not a secret? Why do we highlight that it'll not be leaked here?
Code quote:
the MAC will not be leakedDocumentation/devel/encfiles.rst line 421 at r7 (raw file):
and with the newly generated key. As part of this encryption operation, the MAC is generated and is stored in the ``root_mht_node_mac`` field of the metadata node's header. Since the header will be encrypted, the MAC will not be leaked.
ditto
Code quote:
the MAC will not be leakedDocumentation/devel/encfiles.rst line 515 at r7 (raw file):
one-time keys. The KDK is only used to derive the metadata-node key, thus it produces much less ciphertext than if it would be used to directly encrypt file data. Therefore, the usual NIST limits would be reached much slower.
can we be more precise? do you mean the limit on (encryption) invocations?
Code quote:
usual NIST limitsSigned-off-by: Dmitrii Kuvaiskii <[email protected]>
There was a problem hiding this comment.
Reviewable status: 3 of 11 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)
Documentation/devel/encfiles.rst line 16 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
-> and?
Done.
Documentation/devel/encfiles.rst line 144 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
what about the key for the data node (from
rdrand)?
Done.
Documentation/devel/encfiles.rst line 179 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
-> encrypted part?
No, I think my writing is correct. See the part below starting with "Encrypted files on host storage are represented as a string of 4KB chunks ...". There I explain that there is an encrypted header, which consists of two parts (the metadata and the file contents). Here, metadata_decrypted concerns only the metadata, so it is the "decrypted part of the encrypted header".
Documentation/devel/encfiles.rst line 195 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Why we call it "encrypted header"? It's a bit confusing that it's noted in the diagram that the header of metadata node is in plaintext
Done. Good catch, I rephrased so that it is clear only the first 58 bytes are in plaintext (what I call the "plaintext header").
Documentation/devel/encfiles.rst line 231 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
->
pf_context?
Done.
Documentation/devel/encfiles.rst line 293 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
corresponding
Done.
Documentation/devel/encfiles.rst line 332 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
-> in?
Done.
Documentation/devel/encfiles.rst line 403 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I think the MAC is not a secret? Why do we highlight that it'll not be leaked here?
Hm, good point, technically there's no need to encrypt the MAC. I just removed this sentence, but if you have other ideas how to fix this, please share.
Documentation/devel/encfiles.rst line 421 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
ditto
Done.
Documentation/devel/encfiles.rst line 515 at r7 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
can we be more precise? do you mean the limit on (encryption) invocations?
Done. Yes, that's what is meant here. Better now?
There was a problem hiding this comment.
Reviewed 8 of 8 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners
Documentation/devel/encfiles.rst line 179 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No, I think my writing is correct. See the part below starting with "Encrypted files on host storage are represented as a string of 4KB chunks ...". There I explain that there is an encrypted header, which consists of two parts (the metadata and the file contents). Here,
metadata_decryptedconcerns only the metadata, so it is the "decrypted part of the encrypted header".
Yes, I was confused by the "header" in the text and in the plot while reading this. It's clear now.
Documentation/devel/encfiles.rst line 515 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. Yes, that's what is meant here. Better now?
Yep, thanks!
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, 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):
@donporter Could you please review this PR?
There was a problem hiding this comment.
Reviewed 1 of 11 files at r1, 1 of 10 files at r4, 1 of 9 files at r7, 8 of 8 files at r8, all commit messages.
Reviewable status: all files reviewed, 36 unresolved discussions, 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…
@donporter Could you please review this PR?
Looks great overall. Lots of small suggestions to make things clearer/flow better, but probably ok to publish as-is.
Documentation/index.rst line 142 at r8 (raw file):
implemented and unimplemented features of Gramine, including the lists of available system calls and pseudo-files. - :doc:`devel/encfiles` -- This page has a description of the Encrypted Files
Nit: I would prefer active voice: "This page describes the Encrypted..."
But the bullet above is passive voice (the one below is active). I'd edit the one above for consistency.
Documentation/devel/encfiles.rst line 5 at r8 (raw file):
.. note :: This is a highly technical document intended for crypto practitioners.
cryptography?
Documentation/devel/encfiles.rst line 10 at r8 (raw file):
and closely corresponds to Gramine v1.6. A short introduction to the "protected files" feature as implemented in Intel
Consider moving this paragraph after the following one - first introduce the reader to protected files, then mention the reference.
Documentation/devel/encfiles.rst line 14 at r8 (raw file):
<https://web.archive.org/web/20230401201058/https://www.tatetian.io/2017/01/15/understanding-sgx-protected-file-system/>`__. Gramine provides a feature of :ref:`encrypted-files`, which encrypts files and
I would reword, and remove "provide" (weak verb): "With Gramine's encrypted files feature, Gramine transparently encrypts and decrypts files when the application writes and reads the files, respectively".
Documentation/devel/encfiles.rst line 16 at r8 (raw file):
Gramine provides a feature of :ref:`encrypted-files`, which encrypts files and transparently decrypts them when the application reads or writes them. Integrity- and confidentiality-sensitive files (or whole directories) accessed
I would consider introducing the mount first, perhaps with the user/configurer as the actor: "To use the encrypted files feature, the user must place integrity- and confidentiality-sensitive files (or whole directories) under the "encrypted" FS mount in the Gramine manifest."
Maybe a tt decoration instead of quotes around "encrypted"?
Documentation/devel/encfiles.rst line 24 at r8 (raw file):
<https://download.01.org/intel-sgx/sgx-linux/2.23/docs/Intel_SGX_Developer_Reference_Linux_2.23_Open_Source.pdf>`__). Each encrypted file is encrypted separately, i.e. Gramine employs file-level
Comma needed after "i.e."
Documentation/devel/encfiles.rst line 25 at r8 (raw file):
Each encrypted file is encrypted separately, i.e. Gramine employs file-level encryption and not block-level encryption. Each "encrypted" FS mount can have a
Can or does? Since this is a highly technical document, maybe you do want to say the precise thing here.
Documentation/devel/encfiles.rst line 34 at r8 (raw file):
Encrypted files are primarily used with TEEs (Trusted Execution Environments) like Intel SGX, i.e. with :program:`gramine-sgx`. However, for debug purposes,
ditto re comma
Documentation/devel/encfiles.rst line 34 at r8 (raw file):
Encrypted files are primarily used with TEEs (Trusted Execution Environments) like Intel SGX, i.e. with :program:`gramine-sgx`. However, for debug purposes,
debugging
Documentation/devel/encfiles.rst line 37 at r8 (raw file):
encrypted files are also functional in :program:`gramine-direct`. Security guarantees of encrypted files
Consider "properties"?
Documentation/devel/encfiles.rst line 55 at r8 (raw file):
- **Rollback/replay attacks after file close**. The user cannot detect whether he has opened an old (but authenticated) version of a file. In other words,
he has -> they have (inclusive language)
Documentation/devel/encfiles.rst line 62 at r8 (raw file):
see more details below). - **Side-channel attacks**. Some file metadata, such as file name, file size, access time, access patterns (e.g., which blocks are read/written), etc. is
Don't need "etc" with "such as". Consider rephrasing: "Some file metadata is not confidentiality-protected, such as..."
Documentation/devel/encfiles.rst line 63 at r8 (raw file):
- **Side-channel attacks**. Some file metadata, such as file name, file size, access time, access patterns (e.g., which blocks are read/written), etc. is not confidentiality-protected. This could be used by attackers to gain
Nit: Consider expanding "This"
Documentation/devel/encfiles.rst line 69 at r8 (raw file):
.. note :: There is an effort to improve rollback/replay attack protection in Gramine.
Nit: improve -> add?
Documentation/devel/encfiles.rst line 99 at r8 (raw file):
There are several reasons for this decoupling: - Historical reason -- to ease the porting effort from Intel SGX SDK.
Does it also facilitate bugfix sharing?
Documentation/devel/encfiles.rst line 106 at r8 (raw file):
The application code is *not* aware of encrypted files. Applications treat encrypted files just like regular files, e.g. apps open file descriptors (FDs),
comma after "e.g.". You could probably appeal to similar behavior in encrypted file systems on traditional OSes if you like.
Consider splittling this into a new sentence.
Documentation/devel/encfiles.rst line 120 at r8 (raw file):
.. image:: ../img/encfiles/01_encfiles_datastructs.svg :target: ../img/encfiles/01_encfiles_datastructs.svg :alt: Figure: Relations between the app, the Gramine FS code, the Gramine glue code and the generic encrypted-files code
Nit: Period at end?
Documentation/devel/encfiles.rst line 123 at r8 (raw file):
The diagram above shows the relations between the application, the Gramine FS code, the Gramine glue code and the generic encrypted-files code. Here the
Comma after "Here", or just remove it.
Documentation/devel/encfiles.rst line 126 at r8 (raw file):
``libos_encrypted_file`` data structure is hosted in the glue code, and the ``pf_context`` data structure is hosted in the generic encrypted-files code. The KDK is installed through Gramine interfaces into the ``libos_encrypted_key``
Make sure to expand KDK (if not give a link to a definition) on first real use. Enough text has passed since the mention above that it is worth unpacking.
Documentation/devel/encfiles.rst line 130 at r8 (raw file):
code. Also, the glue code opens a host file via Gramine's PAL interfaces and saves the reference to it into ``pal_handle``, which is copied into ``host_file_handle`` in encrypted-files code. With these two fields, plus the
To give the reader a better sense of where you are going, consider moving this last sentence before the "The KDK..." sentence like:
In addition to the standard file-system callbacks and PAL interfaces in Gramine, the encrypted FS requires one additional configuration option for the key derivation key (KDK).
Documentation/devel/encfiles.rst line 145 at r8 (raw file):
the required PRF (Pseudorandom Function) instantiated by AES-128-CMAC. - Keys for node encryption (i.e., keys stored in the MHT nodes) are generated
Expand MHT on first use
Documentation/devel/encfiles.rst line 155 at r8 (raw file):
- The crypto library used is mbedTLS, frequently updated by Gramine maintainers to be of the latest released version.
Remove "of"
Documentation/devel/encfiles.rst line 171 at r8 (raw file):
.. image:: ../img/encfiles/02_encfiles_representation.svg :target: ../img/encfiles/02_encfiles_representation.svg :alt: Figure: Representation of an encrypted file on host storage and inside the SGX enclave
Nit: Period at end?
Documentation/devel/encfiles.rst line 197 at r8 (raw file):
1. The plaintext header, occupying bytes 0-57. The header contains a magic string, a major version of the encrypted-files protocol, a minor version, a nonce for KDF (Key Derivation Function, explained later) and a MAC
Is it possible to link to the section that discusses these terms? I'm guessing this is an artifact of editing, since some of these have already been mentioned at this point.
Documentation/devel/encfiles.rst line 248 at r8 (raw file):
.. image:: ../img/encfiles/03_encfiles_layout.svg :target: ../img/encfiles/03_encfiles_layout.svg :alt: Figure: Merkle Hash Tree of an encrypted file and file layout on host storage
Nit; period?
Documentation/devel/encfiles.rst line 253 at r8 (raw file):
constitute a single encrypted file, as well as the on-disk data layout of the same file. This diagram visualizes the difference between logical and physical node numbers: the former are used to calculate the offsets in plaintext file
file,
Documentation/devel/encfiles.rst line 290 at r8 (raw file):
.. image:: ../img/encfiles/04_encfiles_write_less3k.svg :target: ../img/encfiles/04_encfiles_write_less3k.svg :alt: Figure: Write flow for an encrypted file with size less than 3KB
Nit: period
Documentation/devel/encfiles.rst line 326 at r8 (raw file):
.. image:: ../img/encfiles/05_encfiles_read_less3k.svg :target: ../img/encfiles/05_encfiles_read_less3k.svg :alt: Figure: Read flow for an encrypted file with size less than 3KB
Nit: period
Documentation/devel/encfiles.rst line 343 at r8 (raw file):
``metadata_node``. The actual file contents are stored in ``file_data`` which is located in the encrypted-header part of the metadata node. Thus, Gramine must decrypt the encrypted header. To obtain the same key as was used for encryption,
s/as/that/
Documentation/devel/encfiles.rst line 367 at r8 (raw file):
--------------------------- Below are the flows for the general case of encrypted-file I/O, i.e. for files
comma after i.e.
Documentation/devel/encfiles.rst line 372 at r8 (raw file):
.. image:: ../img/encfiles/06_encfiles_write_greater3k.svg :target: ../img/encfiles/06_encfiles_write_greater3k.svg :alt: Figure: Write flow for an encrypted file with size greater than 3KB
Nit: period
Documentation/devel/encfiles.rst line 443 at r8 (raw file):
.. image:: ../img/encfiles/07_encfiles_write_greater3k_general.svg :target: ../img/encfiles/07_encfiles_write_greater3k_general.svg :alt: Figure: Generic write flow for an encrypted file with size greater than 3KB
Nit: punctuation
Documentation/devel/encfiles.rst line 450 at r8 (raw file):
.. image:: ../img/encfiles/08_encfiles_read_greater3k.svg :target: ../img/encfiles/08_encfiles_read_greater3k.svg :alt: Figure: Read flow for an encrypted file with size greater than 3KB
Nit: punctuation
Documentation/devel/encfiles.rst line 485 at r8 (raw file):
.. image:: ../img/encfiles/09_encfiles_read_greater3k_general.svg :target: ../img/encfiles/09_encfiles_read_greater3k_general.svg :alt: Figure: Generic read flow for an encrypted file with size greater than 3KB
Nit: punctuation
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
There was a problem hiding this comment.
Reviewable status: 9 of 11 files reviewed, 15 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 @donporter, @g2flyer, and @kailun-qin)
Documentation/index.rst line 142 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: I would prefer active voice: "This page describes the Encrypted..."
But the bullet above is passive voice (the one below is active). I'd edit the one above for consistency.
Done
Documentation/devel/encfiles.rst line 8 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
During rebase, change this to
v1.7(released in the meantime).Also, probably it won't be April 2024.
Done
Documentation/devel/encfiles.rst line 5 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
cryptography?
Done
Documentation/devel/encfiles.rst line 10 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Consider moving this paragraph after the following one - first introduce the reader to protected files, then mention the reference.
Done
Documentation/devel/encfiles.rst line 14 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
I would reword, and remove "provide" (weak verb): "With Gramine's encrypted files feature, Gramine transparently encrypts and decrypts files when the application writes and reads the files, respectively".
Done
Documentation/devel/encfiles.rst line 16 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
I would consider introducing the mount first, perhaps with the user/configurer as the actor: "To use the encrypted files feature, the user must place integrity- and confidentiality-sensitive files (or whole directories) under the "encrypted" FS mount in the Gramine manifest."
Maybe a tt decoration instead of quotes around "encrypted"?
Done
Documentation/devel/encfiles.rst line 24 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Comma needed after "i.e."
Done.
Documentation/devel/encfiles.rst line 25 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Can or does? Since this is a highly technical document, maybe you do want to say the precise thing here.
Done.
Documentation/devel/encfiles.rst line 34 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
ditto re comma
Done.
Documentation/devel/encfiles.rst line 34 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
debugging
Done.
Documentation/devel/encfiles.rst line 37 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Consider "properties"?
Done
Documentation/devel/encfiles.rst line 55 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
he has -> they have (inclusive language)
Done.
Documentation/devel/encfiles.rst line 62 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Don't need "etc" with "such as". Consider rephrasing: "Some file metadata is not confidentiality-protected, such as..."
Done.
Documentation/devel/encfiles.rst line 63 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: Consider expanding "This"
Done
Documentation/devel/encfiles.rst line 69 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: improve -> add?
Done
Documentation/devel/encfiles.rst line 99 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Does it also facilitate bugfix sharing?
Done.
Documentation/devel/encfiles.rst line 106 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
comma after "e.g.". You could probably appeal to similar behavior in encrypted file systems on traditional OSes if you like.
Consider splittling this into a new sentence.
Done
Documentation/devel/encfiles.rst line 120 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: Period at end?
Done
Documentation/devel/encfiles.rst line 123 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Comma after "Here", or just remove it.
Done.
Documentation/devel/encfiles.rst line 126 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Make sure to expand KDK (if not give a link to a definition) on first real use. Enough text has passed since the mention above that it is worth unpacking.
Done.
Documentation/devel/encfiles.rst line 130 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
To give the reader a better sense of where you are going, consider moving this last sentence before the "The KDK..." sentence like:
In addition to the standard file-system callbacks and PAL interfaces in Gramine, the encrypted FS requires one additional configuration option for the key derivation key (KDK).
Done
Documentation/devel/encfiles.rst line 145 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Expand MHT on first use
Done.
Documentation/devel/encfiles.rst line 155 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Remove "of"
Done.
Documentation/devel/encfiles.rst line 171 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: Period at end?
Done
Documentation/devel/encfiles.rst line 197 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Is it possible to link to the section that discusses these terms? I'm guessing this is an artifact of editing, since some of these have already been mentioned at this point.
Done. The explanation of KDF used is buried deep inside the next section, so I don't see that an explicit link to the section name would be too useful.
Documentation/devel/encfiles.rst line 248 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit; period?
Done
Documentation/devel/encfiles.rst line 253 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
file,
Done.
Documentation/devel/encfiles.rst line 290 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: period
Done
Documentation/devel/encfiles.rst line 326 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: period
Done
Documentation/devel/encfiles.rst line 343 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
s/as/that/
Done.
Documentation/devel/encfiles.rst line 367 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
comma after i.e.
Done.
Documentation/devel/encfiles.rst line 372 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: period
Done
Documentation/devel/encfiles.rst line 443 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: punctuation
Done
Documentation/devel/encfiles.rst line 450 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: punctuation
Done
Documentation/devel/encfiles.rst line 485 at r8 (raw file):
Previously, donporter (Don Porter) wrote…
Nit: punctuation
Done
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
Description of the changes
Add chapter on encrypted files implementation.
Also see #1841, which uses (almost) the same names of data structs and fields.
How to test this PR?
Temporarily enabled ReadTheDocs: https://gramine.readthedocs.io/en/dimakuv-docs-encrypted-files/devel/encfiles.html
This change is