-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: protected files redesign #371
Comments
One immediate comment: we need backwards-compatibility in our manifest file. While I agree that per-mount properties ( Another immediate comment: how can we support an |
We also need to carefully design how keys are deployed - they are never stored in manifest (outside of testing). While PF can be a generic feature, some of the mentioned keys (e.g. mrenclave_key) are SGX specific |
Yeah, if arbitrary files can be protected, then probably the best way to emulate them is by separate mountpoints. However, the "right way of doing things" would probably involve a single mount of a whole directory, and optionally a list of allowed files (if we want to only allow some files from that directory): # old way:
sgx.protected_files = ["file:pf/x", "file:pf/y", "file:pf/z"]
# new way:
[fs.mount.pf]
type = "chroot"
path = "/pf"
uri = "file:/pf"
protected = true
# and, if we want to allow access to only some files from pf/:
[fs.mount.pf]
allowed_files = ["file:pf/x", "file:pf/y", "file:pf/z"] This suggests that Does that make sense? I guess it would make it harder to mix protected and non-protected files from the same directory (since you mount the whole directory as protected). Do people do that? Should they?
|
Would it be acceptable if instead of keeping the compatibility in Gramine itself we published a tool to automatically translate old manifests to the new format? And mention the tool if parsing fails for someone. |
This is exactly correct, the Protected Files module (https://github.com/gramineproject/gramine/tree/master/Pal/src/host/Linux-SGX/protected-files) was specifically designed in such a way as to be independent from the rest of the Gramine codebase (bar a couple handy macros). For example, this Protected Files module constitutes the bulk of the So I'm all in for moving Protected Files under |
I created an example dir hierarchy. I think it reflects a typical application well, and it covers all things that we want to support with allowed/trusted/protected files.
Legend:
Obviously, files under Notice how it is typical to mix allowed, trusted and/or protected files under one directory. Also notice how it is typical to mix allowed, trusted and/or protected sub-directories under one directory. |
Regarding the manifest syntax. I highly dislike the hack of I would propose the following manifest syntax, using my example dir tree above (I skip
|
Regarding the renaming feature:
|
While doing such a big rework, let's consider some breaking changes:
|
I agree that maybe we can just rename everything :) I dislike the historical
Obviously, The nice thing about this proposal is that it is obvious and uses well-defined terms. The bad thing is that we won't be able to introduce e.g. another encryption scheme in addition to the one (SGX Protected Files) we have currently. But maybe this is not a bad thing after all -- the less options we have, the better. But if this proposal is too dramatic, I'm fine with keeping the list of keywords and just renaming them like Michal suggests.
Yes. Nobody understands what URIs are and why Gramine needs them, including myself. I cannot imagine that Gramine will at some point implement a
Yes, "mode" sounds better. |
I like the idea with 2 booleans, although it will be quite a breaking change :) But in the long term it should be more logical for new users. |
|
Some comments (from me, but based on discussion with @mkow @boryspoplawski @woju): I don't think boolean flags are a good idea, they obscure the fact that we have 3 distinct mechanisms, each with its own quirks (protected files use a key which you can specify, trusted files are read-only). I like the idea of I think these mechanisms are confusing right now because 1. the names are bad, 2. you configure them as three separate "whitelists" on top of the mounts, instead of making a decision per mounted file/directory. So maybe converting them to mounts would help. There's also the matter of compatibility and interpreting the old manifest in a new system. For instance, given the manifest in the current syntax:
we would have to process
Note the following:
Regarding renaming, I would keep the usual Linux rule, i.e. you can rename files within the same mounted filesystem. If the user wants to rename between 2 protected-file paths, they need to be within the same mounted directory, not two individual mounted files.
Sounds nice for us (a conversion tool could be written in Python!), but much less convenient for the user...
I kind of like the fact that host paths have the |
Another thing (thanks @woju): this will require converting |
We even have an issue for changing it to a list: #23 |
+1, when we discussed it more I changed my mind and don't like the bools idea anymore. It would be viable if we could make all three mount modes have the same interface, but there are too many differences the user needs to know right now, as Paweł mentions.
I'd prefer if we had this in key names (e.g. |
I like Borys's suggestion:
I agree with Michal that the I agree with the like-Linux renaming rule. I don't care about the keywords Regarding compatibility, I don't have a good suggestion. I dislike the idea of a Python tool to convert old manifests to new manifests -- every time I used such conversion tools, I ended up converting my files manually. And I think it will the same experience of Gramine users -- they will try to use the tool, the tool will not cover some corner cases or will emit some hard-to-decipher warnings, and the proficient users will simply do the conversion themselves and the less skilled users will create GitHub issues. I would definitely want to keep compatibility inside Gramine itself, i.e., Gramine manifest loader parses deprecated |
How do we go about auto-generated
|
But shouldn't this prefix be in the key names? Here it suggests that there could be something different in its place, i.e. now it suggests that the user chooses to serve this file from the host.
Yeah, probably something like that. |
I'm not sure if I like it, the nesting is pretty deep. It's a table, nested in an array, nested in a table, nested in an array (since the mounts will be an array-of-tables). Notice that it's no longer even possible to write in the section format. So I think I'd prefer keeping trusted files as a global array. [[fs.mount]]
path = "/"
uri = "file:./"
shielding = "trusted"
[fs]
trusted_files = [
{ uri = "file:/tr_script", sha256 = "..."},
{ uri = "file:/...", sha256 = "..."},
] |
@pwmarcz Just to make sure I understand this and we are on the same page, does it mean renaming is just not permitted? |
No, it is permitted, but only inside the same mount (same as it works on Linux). |
Okay. At least for the failure we encountered with the workload, it should work I guess. |
|
Defining allowed, trusted and protected files via mount points solves #359 so I am closing it. Another question I have is, how do we handle symbolic links - within the same mount and crossing mount borders? |
@lead4good Symlinks are handled purely on LibOS layer and they merely point to another location, so they do not affect protected/trusted files handling |
Going a bit deeper in terms of implementation: I noticed that currently only PAL does crypto (e.g. is linked against mbedtls), and generally PAL performs most of the validation, so that results returned from PAL can be trusted. Implementing protected files entirely in LibOS seems to go against that. I see several choices:
So far the solution number 3 seems to be the most straightforward to me. It looks like a big improvement in terms of code separation:
|
I dislike solution number 3. What would we do with Trusted Files? Have a separate set of functions for them too? And how do we implement Also, with solution number 3 we grow the PAL API and its complexity. I think this goes against Gramine design: PAL is supposed to be simple and stateless, and LibOS is supposed to implement all host-agnostic generic functionality. So to me it makes more sense to lift all PF logic to the LibOS layer. When we add a new PAL, e.g. Windows-SGX, having PFs in LibOS will be a much cleaner approach than re-implementing the Linux-SGX logic in the new Windows-SGX PAL. So I'm actually voting for solution number 1. |
I think so, although trusted files, being read-only, have a different set of operations.
Renaming a protected file means (1) renaming the host file and (2) modifying its header. So it would also be a separate operation that invokes
My idea was to keep that in the "common" part of PAL, not the host-specific part. But you're right, it does look like going against the design. I guess what I'm proposing is a set of functions that wrap PAL handle operations. These functions might be a part of PAL API, but they might also be in LibOS, the difference is not big.
I'm almost convinced, but what worries me is that solution number 1 is breaking the precedent that "only PAL does crypto". Is that meaningful? Do you have any comment on that? Also, do you propose the same for trusted files? I think the situation is very similar. |
I see no reason to limit crypto operations to PAL only. Realistically speaking -- Gramine will be 99% used for TEEs, and limiting the main component of Gramine to not use crypto makes no sense. I would actually push for moving more and more crypto into the LibOS layer, so that writing new backends (vendor-specific TEE PALs) would be a pretty trivial effort. In the worst case, we can hide all this crypto under a compile-time switch (
I'm not sure what exactly you mean by "the difference is not big". Expanding the PAL API makes a huge difference -- I'm currently in the process of writing a new PAL, and it's already a pain in the neck with our APIs. |
Yes, I propose to move all allowed/trusted/protected files logic into the LibOS layer. |
By "writing a new PAL", do you mean not reusing any code from |
I mean writing a new set of headers/sources under |
OK, then what I had in mind was not adding anything to I'll try it out with a LibOS implementation, though. |
Ah, I see. This could be done, yes, and could help. But now we unnecessarily bloat and complicate the PAL component. To me, it sounds more reasonable to expand the LibOS codebase. |
I implemented most of the functionality, allowing me to explore the design better. Here is a detailed proposal for the feature. Please comment, I'm close to submitting initial PRs. Keep in mind that this is an incremental change. I'm not planning to do anything with trusted files, remove URI prefixes, etc. The scope is mostly limited to the current features of protected files (except for bugs/features mentioned in the original post). Mounting encrypted filesProtected files are renamed to encrypted. To mount a file or directory as encrypted, use fs.mounts = [
{ type = "encrypted", path = "/mnt/pf", uri = "file:pf" },
] Why? It makes the most sense to implement KeysEncrypted files have a wrap key, identified by name. The default key is called fs.mounts = [
{ type = "encrypted", path = "/mnt/pf/default", uri = "file:pf/default" },
{ type = "encrypted", path = "/mnt/pf/custom", uri = "file:pf/custom", key = "custom" },
] The keys can be specified directly in manifest: fs.keys.default = "00112233445566778899aabbccddeeff"
fs.keys.custom = "ffeeddccbbaa99887766554433221100" They can also be provided from inside Gramine, by writing the key to Special keys (SGX MRENCLAVE and MRSIGNER)You can use special keys provided by the PAL host. Currently, Linux-SGX provides two such keys:
Special keys have names beginning with underscore. If we are running under a host that does not implement a given key, Gramine will start, but it will not be possible to access mounts using this key (file operations will fail with Why? Otherwise, TransitionWhen the feature is launched, Gramine will translate However, this is a big feature. I want to merge a non-feature-complete version earlier, without such compatibility layer, and without mention in documentation (or documented as "experimental"). ImplementationHere's how the implementation looks so far:
|
I like the proposal a lot. However, some open questions:
Do I understand correctly that you want to have at least several commits in Gramine's master branch that will break user manifests? I'm against this. We have many users who track the latest commit of Gramine, and they will spam us with "nothing works" messages. Even if this compatibility layer is too big, we still need it in the same PR as the main PF changes. Alternatively, if you want the PRs to be more easy-to-review, then you could submit two PRs (second PR with a base of the first PR) and we'll review them together and merge them together. |
That's not what I want to do. First, I want to add Then, once |
Checklist for the transition to
Most likely this will be two PRs: one for |
if the rename syscall is still error when we using in gramine ? Maybe if there some plan about when to fix this syscall, which makes me can not introduce the rocksdb into gramine env |
@g302ge Renaming of Protected (= Encrypted) Files is supported now in Gramine, but it is currently not yet advertised, because @pwmarcz is still finalizing this redesign. This redesign should be finished in a couple weeks hopefully, and then all new features will be advertised to users. That said, you can try the latest Gramine with this new redesign. Note that Protected Files are renamed to Encrypted Files. Note also that
Some examples of how Encrypted Files work now:
|
@dimakuv thx for your job working on protected file system. But the |
Hi, I tried to use encrypted fs. When I try to get a list of files in a directory (even an empty one), I get an error, see the log below:
I used the following manifest:
The same situation with the python os.walk, etc How can I fix this problem? |
@pwmarcz Could you please look into this? Doesn't look good:
UPDATE: This was fixed in #564. @Villain88 Try your stuff again. |
Thanks, this case is working now |
I believe we implemented everything that was mentioned in this RFC. Closing. |
Summary
Make protected files configurable per mount, and move handling to LibOS.
Problems
This proposal addresses the following problems:
Bug: Protected files do not support renaming ([Pal/Linux-SGX] Fix renaming issue with protected files #31). This is difficult to fix for the following reasons:
Bug: Protected files in PAL use a broken reference counting scheme
Bug: If host returns the same FD for multiple files, Gramine segfaults (SGX protected files possible vulnerability graphene#2360)
Bug: Path normalization for protected files is buggy (
foo
and./foo
are treated differently)The following problems are NOT addressed by this proposal:
Bug: Protected files are not flushed at
exit
, resulting in data corruption (File corruption using protected files graphene#2663)Bug: Unmapping protected files does not work properly (Unmapping protected files does not work properly #231)
Current design
As an overview, here is the current implementation of protected files.
Manifest: The user specifies
sgx.protected_files
(as well asprotected_{mrenclave,mrsigner}_files
). These are lists of paths (files AND directories) in the host.LibOS: does not know which files are protected, but (based on specified mounts) translates some I/O operations to PAL operations on host paths.
SGX PAL (
db_files.c
): during file operations, checks if the path corresponds to a protected file. If so, forwards the operationsstruct protected_file
is NOT associated with the PAL handle directly, but looked up in a hash table (indexed by path) every time.SGX PAL adapter to protected files (
enclave_pf.[ch]
). This is a module responsible for tracking open protected files.struct protected_file
: describes a protected file path (:warning: either a single file, or a directory!)struct protected file
pf_context_t
, which is set when the file is loadedg_pf_wrap_key
,g_pf_{mrenclave,mrsigner}_key
) and decides to use one of themProtected files module (
protected_files.[ch]
). This is a generic module for handling protected files.pf_context_t
handles (no global map)I marked with⚠️ the parts that I particularly do not like.
Proposed changes
Basically, I propose scrapping everything except for point 5 above.
This sidesteps problems with renaming (it's impossible to rename a file "out of protected files", because it would mean moving it out of a mount). It also allows setting different keys for different path.
Move protected files handling to LibOS. If the protected files are configured by mount, then the LibOS filesystem code is the perfect place to determine whether to encrypt/decrypt a file and what key to use. Also, unlike PAL, it already tracks all files, so a protected file handle could be stored in the inode.
As a bonus, protected files will not be SGX-only, which should make them easier to test.
Move the generic module (part 5 above) to Common. It looks generic enough, and probably doesn't need any major changes to implement the above.
The text was updated successfully, but these errors were encountered: