[LibOS] Support renaming encrypted files

[pwmarcz]

Description of the changes

This adds rename support for encrypted files. The lowest layer is much like the old implementation in #31, but thanks to encrypted files being in LibOS, the issues mentioned there do not appear:

• it's possible to rename a file that is currently open
• it's not possible to rename a file "into" or "out of" encrypted files (since rename works only within a mounted filesystem)
• this shouldn't introduce problems with reference counting

Part of protected files redesign (#371).

Note that this does not affect sgx.protected_files, which still uses the old subsystem (we'll switch once the new one has all the same features).

How to test this PR?

The rename_unlink test is enabled for encrypted files. It tests various scenarios for deleting and renaming a file, possibly overwriting another file, keeping an open handle to the now-unlinked file, etc.

I also performed a manual test using the Python example:

fs.mounts = [
  { type = "encrypted", path = "/enc", uri = "file:enc" },
]
fs.insecure__keys.default = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'

from pathlib import Path
import os

Path('enc/a').write_text('Hello world')
os.rename('enc/a', 'enc/b')
print(Path('enc/b').read_text())

---

This change is 

[dimakuv]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 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 4 at r1:
Could you add the co-author + signed-off-by Sankaranarayanan Venkatasubramanian [email protected]? He did #31 originally.

---

CI-Examples/python/python.manifest.template, line 23 at r1 (raw file):

  { type = "tmpfs", path = "/tmp" },
  { type = "encrypted", path = "/enc", uri = "file:enc" },

Leftover from a debugging session?

---

common/src/protected_files/protected_files.c, line 1271 at r1 (raw file):

    memcpy(pf->encrypted_part_plain.path, new_path, new_path_size);
    pf->need_writing = true;

Hm, what about calling ipf_internal_flush()? This was in the original PR #31.

Unless I'm missing something, we should do this flush operation. Otherwise another process/thread trying to open this file will still see the old path in the PF metadata and will fail (because the new_path of the file doesn't correspond to the old path in the file's metadata).

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 540 at r1 (raw file):

    }

    ret = DkStreamChangeName(enc->pal_handle, new_uri);

Is it always guaranteed that we have a enc->pal_handle? I thought it was possible to have the enc object without the open PAL handle. Couldn't there be a situation when we want to rename an encrypted file that is not actively used?

Compare e.g. with your implementation of a classic chroot_rename() -- there you actually open a temporary PAL handle for the rename-operation sake.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 555 at r1 (raw file):

    }

    enc->uri = new_uri_copy;

What about the old enc->uri? You don't free it. Won't you have a memory leak?

---

LibOS/shim/src/fs/chroot/encrypted.c, line 284 at r1 (raw file):

    ret = chroot_dentry_uri(new, old->inode->type, &new_uri);
    if (ret < 0)
        return 0;

What does this mean? Why do you return success?

---

LibOS/shim/test/regression/test_libos.py, line 606 at r1 (raw file):

        self.assertIn('TEST OK', stdout)

    @unittest.skip('Protected files do not support renaming yet')

This comment is confusing now. I understand that here we mean "PFs as implemented in Linux-SGX PAL", and below we mean "PFs (Encrypted Files) as implemented in LibOS". Could you add some explanation to this comment, like (as implemented in Linux-SGX PAL)?

[dimakuv]

Reviewable status: all files reviewed, 8 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):

it's not possible to rename a file "into" or "out of" encrypted files (since rename works only within a mounted filesystem)

Actually, isn't this a general problem that our rename works only within a single mount? The issue is that in a normal OS, a single mount is a huge beast covering the whole /, but in Gramine, a single mount is typically a sub-dir like /etc or /tmp or <current-working-dir>. So the unmodified application may try to rename() from e.g. /tmp into a /etc, which would work in a normal OS but will fail in Gramine -- and nothing could be done about it...

---

[pwmarcz]

Reviewable status: 1 of 7 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

it's not possible to rename a file "into" or "out of" encrypted files (since rename works only within a mounted filesystem)

Actually, isn't this a general problem that our rename works only within a single mount? The issue is that in a normal OS, a single mount is a huge beast covering the whole /, but in Gramine, a single mount is typically a sub-dir like /etc or /tmp or <current-working-dir>. So the unmodified application may try to rename() from e.g. /tmp into a /etc, which would work in a normal OS but will fail in Gramine -- and nothing could be done about it...

Unfortunately I don't see a non-hacky way of doing such "cross-mount" renames, especially if we want to keep open handles for these files (which we support now), or do something more complicated like renaming directories (which we don't support yet).

But I'm not sure if this will be a common use-case: you would need two directories that are read-write for the application, not something like /etc or /usr which will be read-only. /tmp is also not a good example, since on normal systems it's usually also a separate mount.

---

---

-- commits, line 4 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you add the co-author + signed-off-by Sankaranarayanan Venkatasubramanian [email protected]? He did #31 originally.

OK. I added a squash commit.

---

common/src/protected_files/protected_files.c, line 1271 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Hm, what about calling ipf_internal_flush()? This was in the original PR #31.

Unless I'm missing something, we should do this flush operation. Otherwise another process/thread trying to open this file will still see the old path in the PF metadata and will fail (because the new_path of the file doesn't correspond to the old path in the file's metadata).

Yeah, I think it makes sense to flush here. Read/write operations do not flush, but this one is different because it can render the file unopenable.

Unfortunately there is still a window between updating the header and actually renaming the file, I don't think we can get rid of that. But this is generally the problem with protected files, concurrent accesses are not going to work well.

BTW: this only matters for multiple processes. Multiple threads use the same pf object, and take a lock.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 540 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is it always guaranteed that we have a enc->pal_handle? I thought it was possible to have the enc object without the open PAL handle. Couldn't there be a situation when we want to rename an encrypted file that is not actively used?

Compare e.g. with your implementation of a classic chroot_rename() -- there you actually open a temporary PAL handle for the rename-operation sake.

Yes, enc object doesn't have to be open, but all these functions (flush, read, write, rename...) operate on enc objects that are open. There are two states:

• open: enc->pf and enc->pal_handle present, enc->use_count > 0,
• closed: both NULL, enc->use_count == 0.

The caller (chroot_encrypted_rename) ensures the file is open (at least temporarily) by calling encrypted_file_get/put. It's done this way because we need to keep exactly 1 handle per file (so, unlike chroot_rename, we cannot create a separate one).

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 555 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What about the old enc->uri? You don't free it. Won't you have a memory leak?

Right, I must have lost it in refactoring. Fixed.

---

LibOS/shim/src/fs/chroot/encrypted.c, line 284 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What does this mean? Why do you return success?

Oh, that's a dumb typo... Fixed.

---

LibOS/shim/test/regression/test_libos.py, line 606 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This comment is confusing now. I understand that here we mean "PFs as implemented in Linux-SGX PAL", and below we mean "PFs (Encrypted Files) as implemented in LibOS". Could you add some explanation to this comment, like (as implemented in Linux-SGX PAL)?

I intend for the new feature to be consistently called "encrypted files", but right, there's also the library which is still called "protected files". I updated according to your suggestion.

---

CI-Examples/python/python.manifest.template, line 23 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Leftover from a debugging session?

Yes. Fixed.

[dimakuv]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "squash! " and "fixup! " found in commit messages' one-liners

a discussion (no related file):

But I'm not sure if this will be a common use-case: you would need two directories that are read-write for the application, not something like /etc or /usr which will be read-only. /tmp is also not a good example, since on normal systems it's usually also a separate mount.

Yes, I know, my examples suck. But you got my point. Anyway, let's finish this discussion, as you think that such support would be hacky.

---

---

common/src/protected_files/protected_files.c, line 1271 at r1 (raw file):

Unfortunately there is still a window between updating the header and actually renaming the file, I don't think we can get rid of that. But this is generally the problem with protected files, concurrent accesses are not going to work well.

Yes, that's true.

[meithecatte]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "squash! " and "fixup! " found in commit messages' one-liners

---

common/src/protected_files/protected_files.c, line 1271 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Unfortunately there is still a window between updating the header and actually renaming the file, I don't think we can get rid of that. But this is generally the problem with protected files, concurrent accesses are not going to work well.

Yes, that's true.

I suppose we could fix that by starting a dedicated "file server" enclave, microkernel-style. You could even coordinate the enclaves such that a file being accessed by only one process is owned by that process and can be manipulated directly.

Not sure how much that'd affect performance, though. Seems like only open/close would suffer in the uncontended case? Such an implementation probably wouldn't be too straightforward, though.

[mkow]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "squash! " and "fixup! " found in commit messages' one-liners

---

common/src/protected_files/protected_files.c, line 1271 at r1 (raw file):

Previously, NieDzejkob (Jakub Kądziołka) wrote…

I suppose we could fix that by starting a dedicated "file server" enclave, microkernel-style. You could even coordinate the enclaves such that a file being accessed by only one process is owned by that process and can be manipulated directly.

Not sure how much that'd affect performance, though. Seems like only open/close would suffer in the uncontended case? Such an implementation probably wouldn't be too straightforward, though.

@niedzejkob It's a hard topic, @pwmarcz already tried to tackle this up, but decided to leave it for later. See gramineproject/graphene#2267. He should be able to elaborate more about the technical problems with this.

[pwmarcz]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "squash! " and "fixup! " found in commit messages' one-liners

---

common/src/protected_files/protected_files.c, line 1271 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

@niedzejkob It's a hard topic, @pwmarcz already tried to tackle this up, but decided to leave it for later. See gramineproject/graphene#2267. He should be able to elaborate more about the technical problems with this.

Yes. The attempt linked by @mkow is actually more complicated: I wanted to have something that works locally for the uncontested case. See also gramineproject/graphene#2158 for more background.

But the end result is that I'm really not proud of this: it ended up way too complicated, and is basically abandoned for now.

The "server" architecture you mentioned is done for POSIX locks implementation (fcntl); the main process works as a server and other processes always refer to it. It still needs some improvement (see https://github.com/oscarlab/graphene/issues/2517) but I think this is basically the way to go for many "multi-process" features.

Still, I wouldn't touch this until we have a good use case that we want to enable.

[mkow]

Reviewed 1 of 7 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

common/src/protected_files/protected_files.h, line 264 at r2 (raw file):

/*!
 * \brief Rename a PF

missing period

---

common/src/protected_files/protected_files.h, line 267 at r2 (raw file):

 *
 * \param [in] pf PF context
 * \param [in] new_path New path

no space after \param + please align (basically, needs an update to our new, more consistent doxycomment style)

[pwmarcz]

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: ITL), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

common/src/protected_files/protected_files.h, line 264 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

missing period

Done.

---

common/src/protected_files/protected_files.h, line 267 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

no space after \param + please align (basically, needs an update to our new, more consistent doxycomment style)

I updated this whole comment to our new style (and omitted the \return PF status as useless, it's obvious from the return type).

[mkow]

Reviewed 1 of 1 files at r3, 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

[dimakuv]

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[RodgerZhu]

Hi, when users append their data to the protected file via "O_APPEND" way, will the whole file be encrypted again? or just the part of new data?
If one protected file is large, for example 5GB, can Gramine just encrypted the file very fast?

[dimakuv]

@RodgerZhu Only the appended part will be encrypted. Everything shouldn't be re-encrypted. But because Protected Files use Merkle Hash Trees (MHT), some parts of the file will be re-hashed (but not re-encrypted).