[LibOS,Pal/Linux-SGX] Replace old protected files subsystem

[pwmarcz]

Signed-off-by: Paweł Marczewski [email protected]

Description of the changes

This change replaces the protected files implemented in Linux-SGX PAL by the new encrypted files subsystem in LibOS, implemented in previous commits.

• Remove old implementation in SGX PAL (protected-file handles, reference counting, setting the key etc)
• Change the old user-facing interfaces (sgx.protected_files, /dev/attestation/protected_files_key) to control encrypted files, add deprecation warnings to them
• Add documentation for the new encrypted files syntax

Background: #371

How to test this PR?

• LibOS/shim/test/regression still has various tests for both the "protected files" and "encrypted files" syntax,
• LibOS/shim/test/fs has a suite of protected-files tests that have been switched to the new syntax.

---

This change is 

[pwmarcz]

Reviewable status: 0 of 28 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

---

Documentation/manifest-syntax.rst line 903 at r1 (raw file):

   fail to do so correctly in more complicated cases (e.g. when a single host
   file belongs to multiple mounts). It is recommended to rewrite all usages of
   this syntax to ``type = "encrypted"`` mounts.

Basically, given this:

fs.mounts = [
  { uri = "file:x", path = "/x1" },
  { uri = "file:x", path = "/x2" },
  { uri = "file:y", path = "/y1" },
  { uri = "file:y", path = "/y2" },
]

sgx.protected_files = [ "file:x/pf", "file:y/pf" ]

We currently produce this:

fs.mounts = [
  { uri = "file:x", path = "/x1" },
  { uri = "file:x", path = "/x2" },
  { uri = "file:y", path = "/y1" },
  { uri = "file:y", path = "/y2" },

  { type = "encrypted", uri = "file:x/pf", path = "/x2/pf" },
  { type = "encrypted", uri = "file:y/pf", path = "/y2/pf" },
]

Where a more correct (but more complicated) version would produce this (note that the order is also important, in the general case:

fs.mounts = [
  { uri = "file:x", path = "/x1" },
  { type = "encrypted", uri = "file:x/pf", path = "/x1/pf" },

  { uri = "file:x", path = "/x2" },
  { type = "encrypted", uri = "file:x/pf", path = "/x2/pf" },

  { uri = "file:y", path = "/y1" },
  { type = "encrypted", uri = "file:y/pf", path = "/y1/pf" },

  { uri = "file:y", path = "/y2" },
  { type = "encrypted", uri = "file:y/pf", path = "/y2/pf" },
]

Is this important? Should I attempt to improve this?

---

LibOS/shim/test/fs/test_pf.py line 96 at r1 (raw file):

    # overrides TC_00_FileSystem to change input dir (from plaintext to encrypted)
    # doesn't work because encrypted files do not support truncation (and the test opens an
    # existing, non-empty file with O_TRUNC)

FYI: protected files do not support truncation, and implementing this seems non-trivial (I don't have enough understanding of the internal file format to do it right now). The previous implementation "solved" this exact case by always creating a new protected-file whenever O_CREAT was specified (even if we were supposed not to truncate it).

I thought of some hacky solutions, but they don't seem viable:

• We could handle open with O_TRUNC and create a new file, but we keep a single reference-counted handle, so by the time user invokes open, we might have opened the file already.

• We could handle the special case of truncating file to 0 size by "resetting" the current protected-file object (destroying it and creating it again), but it complicates error handling (if we fail to create the file again, we don't know how to recover).

[dimakuv]

Reviewed 26 of 27 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 32 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

a discussion (no related file):
How should we go with Examples and GSC repos? I guess like this:

• After the Gramine release that incorporates these changes (I guess the upcoming v1.2), the Examples can be fixed to use this new syntax
• It's a bit more complicated with GSC: do we want to keep GSC working with e.g. v1.1, or do we just allow v1.2 and higher? I guess we should have the same policy as with the Examples -- that GSC is only guaranteed to work with the latest release and higher.

---

a discussion (no related file):
A follow-up PR would also fix:

1. RA-TLS
2. Secret Provisioning
3. CI-Examples (ra-tls-...)

Maybe it would be enough to only fix 1, because 2 and 3 build on top of 1 (so these low-level details are not exposed there).

Similarly, all CI-Examples should be also fixed in a follow-up PR.

---

a discussion (no related file):
I hope this FS rewrite will be the last manifest-syntax change in Gramine. These deprecations are annoying to deal with.

---

a discussion (no related file):
Awesome!

I guess a couple more PRs are left:

• Move GSC to newer syntax
• Move CI-Examples to newer syntax
• Move Examples to newer syntax
• Move RA-TLS and Secret Provisioning libraries to newer syntax
• Similar refactoring of Trusted Files from PAL into LibOS

---

---

Documentation/attestation.rst line 218 at r2 (raw file):

.. note::
   Previously, ``/dev/attestation/protected_files_key`` was used for setting the
   wrap key, and Gramine still supports that file for backward compatibility.

the default wrap key

---

Documentation/manifest-syntax.rst line 903 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Basically, given this:

fs.mounts = [
  { uri = "file:x", path = "/x1" },
  { uri = "file:x", path = "/x2" },
  { uri = "file:y", path = "/y1" },
  { uri = "file:y", path = "/y2" },
]

sgx.protected_files = [ "file:x/pf", "file:y/pf" ]

We currently produce this:

fs.mounts = [
  { uri = "file:x", path = "/x1" },
  { uri = "file:x", path = "/x2" },
  { uri = "file:y", path = "/y1" },
  { uri = "file:y", path = "/y2" },

  { type = "encrypted", uri = "file:x/pf", path = "/x2/pf" },
  { type = "encrypted", uri = "file:y/pf", path = "/y2/pf" },
]

Where a more correct (but more complicated) version would produce this (note that the order is also important, in the general case:

fs.mounts = [
  { uri = "file:x", path = "/x1" },
  { type = "encrypted", uri = "file:x/pf", path = "/x1/pf" },

  { uri = "file:x", path = "/x2" },
  { type = "encrypted", uri = "file:x/pf", path = "/x2/pf" },

  { uri = "file:y", path = "/y1" },
  { type = "encrypted", uri = "file:y/pf", path = "/y1/pf" },

  { uri = "file:y", path = "/y2" },
  { type = "encrypted", uri = "file:y/pf", path = "/y2/pf" },
]

Is this important? Should I attempt to improve this?

No, let's not attempt to improve this.

I haven't seen anyone doing this kind of aliasing (or, as you put it, "a single host file belongs to multiple mounts"). And if they do, this is surely not intentional, but a bug in the manifest.

---

Documentation/manifest-syntax.rst line 648 at r2 (raw file):

    ]

    fs.insecure__keys.[KEY_NAME] = "[16-byte hex value]"

This should be 32 bytes (32 hex symbols). To avoid confusion, maybe re-use the same as in attestation document: [32-character hex value] ?

---

Documentation/manifest-syntax.rst line 657 at r2 (raw file):

Encrypted files were previously known as *protected files*, and some Gramine
tools might still use the old name.

Meh. Do we want renames of tools like gramine-sgx-pf-crypt? Maybe we can just have an alias (symlink) with pf dropped from the name.

---

Documentation/manifest-syntax.rst line 667 at r2 (raw file):

size is limited to 260 bytes.

The ``key_name`` mount parameter specifies the name of the wrap key. If

I never liked the word wrap, but this is what Protected Files always called it. Maybe we should remove the wrap and master words, and simply always write encryption key?

If not, then at least add ...of the wrap encryption key.

---

Documentation/manifest-syntax.rst line 898 at r2 (raw file):

This syntax specified the previous SGX-only protected files. It has been
replaced with ``type = "encrypted"`` mounts.

A ref to encrypted-files?

---

Documentation/manifest-syntax.rst line 908 at r2 (raw file):

::

   fs.insecure__protected_files_key = "[16-byte hex value]"

ditto

---

Documentation/manifest-syntax.rst line 910 at r2 (raw file):

   fs.insecure__protected_files_key = "[16-byte hex value]"

This syntax allowed specifying the wrap key for protected files. It has been

the default wrap key (also, I'd prefer to remove "wrap" everywhere, and then here it will be simply the default encryption key).

---

LibOS/shim/src/fs/shim_fs.c line 429 at r2 (raw file):

 * This function is used for interpreting legacy `sgx.protected_files` syntax as mounts.
 */
static int find_host_file_mount_path(const char* uri, char** full_mount_path) {

Don't we want out_full_mount_path?

---

LibOS/shim/src/fs/shim_fs.c line 440 at r2 (raw file):

    /* Traverse the mount list in reverse: we want to find the latest mount that applies. */
    struct shim_mount* mount;
    LISTP_FOR_EACH_ENTRY_REVERSE(mount, &mount_list, list) {

Aren't you supposed to grab mount_list_lock? If not, then please add a comment that "this func must be called when running single-threaded".

---

LibOS/shim/src/fs/shim_fs.c line 451 at r2 (raw file):

                (!uri[mount_uri_len] || uri[mount_uri_len] == '/')) {
            mount_path = mount->path;
            rel_path = uri + mount_uri_len;

rel_path now starts with / (or it can also be \0). Thus, the name is confusing. Maybe rest_path + a comment?

---

LibOS/shim/src/fs/shim_fs.c line 455 at r2 (raw file):

        }

        /* Special case: this the mount of the current directory, and `uri` is not absolute */

this is

---

LibOS/shim/src/fs/shim_fs.c line 456 at r2 (raw file):

        /* Special case: this the mount of the current directory, and `uri` is not absolute */
        if ((!strcmp(mount_uri, "file:.") || !strcmp(mount_uri, "file:")) &&

Why do we allow this ambiguity in mount->uri? Can we enforce this special mount URI to always be in the form of file:.?

---

LibOS/shim/src/fs/shim_fs.c line 460 at r2 (raw file):

            mount_path = "";
            rel_path = uri + static_strlen(URI_PREFIX_FILE);
            break;

How does this case work? This will end up with smth like mount_path = "" and rel_path = "a/b". Which will return full_mount_path = "a/b", i.e., the returned path is not absolute. Is this normal/expected?

---

LibOS/shim/src/fs/shim_fs.c line 479 at r2 (raw file):

 *
 * NOTE: This covers only the simplest cases. It will not work correctly if a given file was mounted
 * multiple times, or mounted under a path shadowed by another mount.

The second part (or mounted under a path shadowed by another mount) is not relevant, no? These files wouldn't be accessible even in the old version of Gramine.

---

LibOS/shim/src/fs/shim_fs.c line 540 at r2 (raw file):

        }

        free(uri);

What about mount_path? From what I understand, mount_fs() doesn't take ownership of this string.

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 213 at r2 (raw file):

        if (hi < 0 || lo < 0) {
            log_warning("%s: unexpected character encountered", __func__);
            return -1;

Here too -EINVAL

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 223 at r2 (raw file):

int dump_pf_key(const pf_key_t* pf_key, char* buf, size_t buf_size) {
    if (buf_size != sizeof(*pf_key) * 2)
        return -EINVAL;

How can this work? The buffer must end with \0, so you'll need + 1 here.

It looks like this function is never really called? Maybe then remove it?

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 312 at r2 (raw file):

     * TODO: this is deprecated in v1.2, remove two versions later.
     */
    if (strcmp(g_pal_public_state->host_type, "Linux-SGX")) {

Oops. You forgot ! :)

I think we need some tests for this deprecated stuff, I wonder how these bugs could go unnoticed.

---

LibOS/shim/src/fs/chroot/fs.c line 564 at r2 (raw file):

    }
    return 0;
}

Nice, no need for this hack!

---

LibOS/shim/src/fs/dev/attestation.c line 239 at r2 (raw file):

}

static int pfkey_load(struct shim_dentry* dent, char** out_data, size_t* out_size) {

Please rename to smth like deprecated_default_key_load and deprecated_default_key_save.

Otherwise it is easy to confuse with new-style key_load / key_save.

---

LibOS/shim/src/fs/dev/attestation.c line 264 at r2 (raw file):

        *out_data = buf;
        *out_size = sizeof(pf_key) * 2;

Hm, is this correct? There is no NUL byte at the end.

---

LibOS/shim/src/fs/dev/attestation.c line 272 at r2 (raw file):

}

static int pfkey_save(struct shim_dentry* dent, const char* data, size_t size) {

ditto

---

LibOS/shim/src/fs/dev/attestation.c line 283 at r2 (raw file):

    pf_key_t pf_key;
    if (size != sizeof(pf_key) * 2) {

Hm, so it looks like that's how this pseudo-file works, without the NUL byte.

---

LibOS/shim/test/fs/manifest.template line 3 at r2 (raw file):

loader.entrypoint = "file:{{ gramine.libos }}"
libos.entrypoint = "{{ entrypoint }}"
loader.log_level = "trace"

Debug leftover

---

LibOS/shim/test/fs/manifest.template line 18 at r2 (raw file):

  { type = "encrypted", path = "/tmp/pf_output", uri = "file:tmp/pf_output" },
  { type = "encrypted", path = "/mounted/pf_input", uri = "file:tmp/pf_input" },
  { type = "encrypted", path = "/mounted/pf_output", uri = "file:tmp/pf_output" },

Why two paths (/tmp and /mounted/)?

In general, you also have a chroot-typed /mounted path above. I'm confused as to what is what here.

---

LibOS/shim/test/fs/test_pf.py line 96 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

FYI: protected files do not support truncation, and implementing this seems non-trivial (I don't have enough understanding of the internal file format to do it right now). The previous implementation "solved" this exact case by always creating a new protected-file whenever O_CREAT was specified (even if we were supposed not to truncate it).

I thought of some hacky solutions, but they don't seem viable:

• We could handle open with O_TRUNC and create a new file, but we keep a single reference-counted handle, so by the time user invokes open, we might have opened the file already.

• We could handle the special case of truncating file to 0 size by "resetting" the current protected-file object (destroying it and creating it again), but it complicates error handling (if we fail to create the file again, we don't know how to recover).

Not for this PR, but can't we just emulate truncation in a very stupid way, by completely overwriting the contents of the file (by doing write()s and filling with zero if the new size is less than the old size)? The semantics will be a bit weird, but it's better than nothing.

Hm, yeah, we'll need to look into this truncation logic inside the PF crypto code.

---

LibOS/shim/test/fs/test_pf.py line 13 at r2 (raw file):

# TODO: While protected (encrypted) files are no longer SGX-only, the related tools
# (gramine-sgx-pf-crypt, gramine-sgx-tamper) are still part of Linux-SGX PAL. As a result, we are

gramine-sgx-pf-tamper

---

LibOS/shim/test/fs/test_pf.py line 14 at r2 (raw file):

# TODO: While protected (encrypted) files are no longer SGX-only, the related tools
# (gramine-sgx-pf-crypt, gramine-sgx-tamper) are still part of Linux-SGX PAL. As a result, we are
# able to run the tests with other PALs, but only if Gramine was build with SGX enabled.

was built

---

LibOS/shim/test/fs/test_pf.py line 16 at r2 (raw file):

# able to run the tests with other PALs, but only if Gramine was build with SGX enabled.

@unittest.skipUnless(_CONFIG_SGX_ENABLED, 'Protected files tests require SGX to be enabled')

Encrypted files. Also, do you want to change "Protected files" everywhere in Gramine codebase? Then you should start with this file (its name should be now test_ef I guess, and the class should be EncryptedFiles).

[pwmarcz]

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

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

How should we go with Examples and GSC repos? I guess like this:

• After the Gramine release that incorporates these changes (I guess the upcoming v1.2), the Examples can be fixed to use this new syntax
• It's a bit more complicated with GSC: do we want to keep GSC working with e.g. v1.1, or do we just allow v1.2 and higher? I guess we should have the same policy as with the Examples -- that GSC is only guaranteed to work with the latest release and higher.

Yeah, I think our policy was that the Examples repo tracks latest Gramine release; i.e. we should update it after the release.

As for GSC, I guess it makes sense to do the same.

I updated the checklist: #371 (comment)

---

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

A follow-up PR would also fix:

1. RA-TLS
2. Secret Provisioning
3. CI-Examples (ra-tls-...)

Maybe it would be enough to only fix 1, because 2 and 3 build on top of 1 (so these low-level details are not exposed there).

Similarly, all CI-Examples should be also fixed in a follow-up PR.

I updated the checklist: #371 (comment)

---

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Awesome!

I guess a couple more PRs are left:

• Move GSC to newer syntax
• Move CI-Examples to newer syntax
• Move Examples to newer syntax
• Move RA-TLS and Secret Provisioning libraries to newer syntax
• Similar refactoring of Trusted Files from PAL into LibOS

ACK. As mentioned above, I updated the checklist in #371 (comment).

As for trusted files... from architectural point of view, it would be nice to do that, but I don't think the benefit is as big. In the case of protected files there were multiple bugs and missing features.

---

---

Documentation/attestation.rst line 218 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

the default wrap key

Done.

---

Documentation/manifest-syntax.rst line 903 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, let's not attempt to improve this.

I haven't seen anyone doing this kind of aliasing (or, as you put it, "a single host file belongs to multiple mounts"). And if they do, this is surely not intentional, but a bug in the manifest.

Well, our fs tests did it :)

But yeah, I'd also prefer to leave it at that.

---

Documentation/manifest-syntax.rst line 648 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This should be 32 bytes (32 hex symbols). To avoid confusion, maybe re-use the same as in attestation document: [32-character hex value] ?

Right. Done.

---

Documentation/manifest-syntax.rst line 657 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Meh. Do we want renames of tools like gramine-sgx-pf-crypt? Maybe we can just have an alias (symlink) with pf dropped from the name.

And sgx dropped, the tools will no longer be SGX-only. I decided it's out of scope for this PR, but it's on the checklist.

---

Documentation/manifest-syntax.rst line 667 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I never liked the word wrap, but this is what Protected Files always called it. Maybe we should remove the wrap and master words, and simply always write encryption key?

If not, then at least add ...of the wrap encryption key.

OK. Changed to "encryption key" in these two documents.

---

Documentation/manifest-syntax.rst line 898 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

A ref to encrypted-files?

Done.

---

Documentation/manifest-syntax.rst line 908 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.

---

Documentation/manifest-syntax.rst line 910 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

the default wrap key (also, I'd prefer to remove "wrap" everywhere, and then here it will be simply the default encryption key).

Done.

---

LibOS/shim/src/fs/shim_fs.c line 429 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Don't we want out_full_mount_path?

OK (shortened to out_file_path).

---

LibOS/shim/src/fs/shim_fs.c line 440 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Aren't you supposed to grab mount_list_lock? If not, then please add a comment that "this func must be called when running single-threaded".

Right, it's best just to grab it.

---

LibOS/shim/src/fs/shim_fs.c line 451 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

rel_path now starts with / (or it can also be \0). Thus, the name is confusing. Maybe rest_path + a comment?

Refactored: there is a short-lived "rest" variable, and a comment.

---

LibOS/shim/src/fs/shim_fs.c line 455 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

this is

Done.

---

LibOS/shim/src/fs/shim_fs.c line 456 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why do we allow this ambiguity in mount->uri? Can we enforce this special mount URI to always be in the form of file:.?

Generally, we don't require that URIs are in any canonical form (e.g. they could contain multiple slashes). I added "file:" here because I think that actually used to be the implicit root mount (before I changed it to "file:.").

But maybe it's good enough to match "file:." here. Our examples never use either (we rely on the implicit root mount), and I think it's unlikely there are manifests in the wild that mount "file:".

---

LibOS/shim/src/fs/shim_fs.c line 460 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

How does this case work? This will end up with smth like mount_path = "" and rel_path = "a/b". Which will return full_mount_path = "a/b", i.e., the returned path is not absolute. Is this normal/expected?

Yes, this is expected. Gramine doesn't ever resolve relative paths to absolute. I think we decided it's not worth the complexity (we would need to know our CWD on host, and maybe also traverse host symlinks), and actually relative paths can be considered a feature, because they make it possible to move the project directory around (including encrypted files inside).

---

LibOS/shim/src/fs/shim_fs.c line 479 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The second part (or mounted under a path shadowed by another mount) is not relevant, no? These files wouldn't be accessible even in the old version of Gramine.

But it changes the semantics: in the old version, these files would not be visible; in the new version, they will be.

---

LibOS/shim/src/fs/shim_fs.c line 540 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What about mount_path? From what I understand, mount_fs() doesn't take ownership of this string.

Right. Fixed.

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 213 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Here too -EINVAL

Fixed, thanks.

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 223 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

How can this work? The buffer must end with \0, so you'll need + 1 here.

It looks like this function is never really called? Maybe then remove it?

Oh. This is used by /dev/attestation/protected_files_key, but our tests never read this file, only write to it.

I added a test for reading/writing this file to attestation regression test.

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 312 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Oops. You forgot ! :)

I think we need some tests for this deprecated stuff, I wonder how these bugs could go unnoticed.

Yeah... I left it like this so that I could easily test it with gramine-direct, then forgot about it.

Anyway, you're completely right, this lacked coverage. I updated the attestation regression test (and found out that this function actually has an early return that prevented this code from running...)

---

LibOS/shim/src/fs/dev/attestation.c line 239 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please rename to smth like deprecated_default_key_load and deprecated_default_key_save.

Otherwise it is easy to confuse with new-style key_load / key_save.

Done.

---

LibOS/shim/src/fs/dev/attestation.c line 264 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Hm, is this correct? There is no NUL byte at the end.

Yes. These pseudo-files use raw data, not null-terminated strings (and other files in /dev/attestation might actually contain null bytes).

I added a comment.

---

LibOS/shim/src/fs/dev/attestation.c line 272 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.

---

LibOS/shim/src/fs/dev/attestation.c line 283 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Hm, so it looks like that's how this pseudo-file works, without the NUL byte.

Yes.

---

LibOS/shim/test/fs/manifest.template line 3 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Debug leftover

Fixed, sorry.

---

LibOS/shim/test/fs/manifest.template line 18 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why two paths (/tmp and /mounted/)?

In general, you also have a chroot-typed /mounted path above. I'm confused as to what is what here.

The mounted mount was here before, and it is used in some tests (I guess that's in order to check what happens if we reach a file through a different mount path).

I added these /mounted mount here to replicate the previous semantics: adding a file to sgx.protected_files means that it was treated as protected when accessed through all paths.

Now that I think of it, this is a limitation of the new encrypted-files subsystem... if you access the same encrypted file through multiple Gramine paths, they will be treated as different files, and bad things will happen. Our tests happen to work because they don't open both these paths at the same time. I added a warning.

---

LibOS/shim/test/fs/test_pf.py line 96 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Not for this PR, but can't we just emulate truncation in a very stupid way, by completely overwriting the contents of the file (by doing write()s and filling with zero if the new size is less than the old size)? The semantics will be a bit weird, but it's better than nothing.

Hm, yeah, we'll need to look into this truncation logic inside the PF crypto code.

I'm not sure if it's better than nothing - instead of returning an error, we'll actively do the wrong thing (i.e. leave a file with wrong size). For example, it will still fail all reasonable tests of truncate() / O_TRUNC.

---

LibOS/shim/test/fs/test_pf.py line 13 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

gramine-sgx-pf-tamper

Fixed.

---

LibOS/shim/test/fs/test_pf.py line 14 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

was built

Fixed.

---

LibOS/shim/test/fs/test_pf.py line 16 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Encrypted files. Also, do you want to change "Protected files" everywhere in Gramine codebase? Then you should start with this file (its name should be now test_ef I guess, and the class should be EncryptedFiles).

OK, done. The abbreviation I've been using so far is enc_*.

[dimakuv]

Reviewed 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Yeah, I think our policy was that the Examples repo tracks latest Gramine release; i.e. we should update it after the release.

As for GSC, I guess it makes sense to do the same.

I updated the checklist: #371 (comment)

I agree. Thanks for adding to the checklist. Resolved.

---

a discussion (no related file):

from architectural point of view, it would be nice to do that, but I don't think the benefit is as big

But otherwise our manifests are plain ugly and not uniform. I definitely want this to be done, but I understand that this may be a PR for someone else.

---

---

LibOS/shim/src/fs/shim_fs.c line 456 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Generally, we don't require that URIs are in any canonical form (e.g. they could contain multiple slashes). I added "file:" here because I think that actually used to be the implicit root mount (before I changed it to "file:.").

But maybe it's good enough to match "file:." here. Our examples never use either (we rely on the implicit root mount), and I think it's unlikely there are manifests in the wild that mount "file:".

Sounds good to me.

---

LibOS/shim/src/fs/dev/attestation.c line 264 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Yes. These pseudo-files use raw data, not null-terminated strings (and other files in /dev/attestation might actually contain null bytes).

I added a comment.

Ah, right, always forget about it.

---

LibOS/shim/test/fs/manifest.template line 18 at r2 (raw file):

if you access the same encrypted file through multiple Gramine paths, they will be treated as different files, and bad things will happen.

Yeah, not perfect, but also this shouldn't really happen in reality. So this limitation is ok.

---

LibOS/shim/test/fs/README.md line 20 at r3 (raw file):

- `gramine-test pytest -v`

Encrypted file tests assume that the Gramine was built with SGX enabled (see comment in `test_pf.py`).

the Gramine -> Gramine

---

LibOS/shim/test/fs/README.md line 20 at r3 (raw file):

- `gramine-test pytest -v`

Encrypted file tests assume that the Gramine was built with SGX enabled (see comment in `test_pf.py`).

test_pf.py -> test_enc.py

Also, please check other places

But also... We need to draw a line when we use pf/protected and when enc/encrypted. I think your logic is to continue using pf for all deprecated stuff (it will be removed after several months anyway) and enc for all stuff that will stay. Is this correct?

---

LibOS/shim/test/fs/test_pf.py line 96 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I'm not sure if it's better than nothing - instead of returning an error, we'll actively do the wrong thing (i.e. leave a file with wrong size). For example, it will still fail all reasonable tests of truncate() / O_TRUNC.

True, ignore me. We need proper truncation logic inside the PF code.

[pwmarcz]

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

---

LibOS/shim/test/fs/README.md line 20 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

the Gramine -> Gramine

Fixed.

---

LibOS/shim/test/fs/README.md line 20 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

test_pf.py -> test_enc.py

Also, please check other places

But also... We need to draw a line when we use pf/protected and when enc/encrypted. I think your logic is to continue using pf for all deprecated stuff (it will be removed after several months anyway) and enc for all stuff that will stay. Is this correct?

Done. I also grepped for pf, protected etc. - we still have plenty of hits for "protected files", but they should be accounted for by the checklist I made.

Yes, that's roughly the idea, to keep using "protected files" for the deprecated feature... although I think that in the end, it might make sense to keep the protected_files name also for the core library in common. It will be a pain to rename everything in that module, and this way, there'll be a clear separation line ("pf" is the core library, "enc" is the feature in Gramine).

[dimakuv]

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

---

LibOS/shim/test/fs/README.md line 20 at r3 (raw file):

("pf" is the core library, "enc" is the feature in Gramine)

I like that! A nice way to explain this seeming inconsistency in naming :)

[mkow]

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

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I hope this FS rewrite will be the last manifest-syntax change in Gramine. These deprecations are annoying to deal with.

Unfortunately I don't think so, at least for some time. We should rather learn how to live better with them ;)

---

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

from architectural point of view, it would be nice to do that, but I don't think the benefit is as big

But otherwise our manifests are plain ugly and not uniform. I definitely want this to be done, but I understand that this may be a PR for someone else.

Yup, IMO we have to do this also for trusted files (including renaming them, as in that refactoring proposal issue).

---

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I updated the checklist: #371 (comment)

There's more to update there, read e.g. common/src/protected_files/README.rst: "Protected files (PF) are a type of files that can be specified in the manifest (SGX only).".

---

---

Documentation/manifest-syntax.rst line 655 at r4 (raw file):

specific host path

This change is wrong, and the original was correct. It's not the host path which is verified (because why would it?), it's the path inside Gramine virtual filesystem (to which the file has to be bound).

---

Documentation/manifest-syntax.rst line 669 at r4 (raw file):

data loss may occur

Why? Wouldn't open() via other paths get rejected?
If not, then PFs have to have a list of valid names inside (I think that was actually the case?), but I don't see how this could cause corruption, and moreover, this feature would seem broken in the first place then.

[pwmarcz]

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

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

There's more to update there, read e.g. common/src/protected_files/README.rst: "Protected files (PF) are a type of files that can be specified in the manifest (SGX only).".

OK, I updated that file to describe the current state better.

(As said elsewhere, I want to keep the name "protected files" for that library, but the library also talks about the feature as implemented in Gramine).

---

---

Documentation/manifest-syntax.rst line 655 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

specific host path

This change is wrong, and the original was correct. It's not the host path which is verified (because why would it?), it's the path inside Gramine virtual filesystem (to which the file has to be bound).

But the file header contains the host path (PAL URI without file: prefix).

That's how it always worked. The path inside Gramine's virtual filesystem is irrelevant, and in the previous implementation, the SGX PAL didn't even know it (and could in theory even open a file that wasn't mounted in Gramine's virtual filesystem).

---

Documentation/manifest-syntax.rst line 669 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

data loss may occur

Why? Wouldn't open() via other paths get rejected?
If not, then PFs have to have a list of valid names inside (I think that was actually the case?), but I don't see how this could cause corruption, and moreover, this feature would seem broken in the first place then.

Again, open() via different Gramine paths is possible, and our tests even verify that (without triggering the issue, because they don't keep the file open via both paths at the same time...) We only verify the host path.

[pwmarcz]

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

---

Documentation/manifest-syntax.rst line 655 at r4 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

But the file header contains the host path (PAL URI without file: prefix).

That's how it always worked. The path inside Gramine's virtual filesystem is irrelevant, and in the previous implementation, the SGX PAL didn't even know it (and could in theory even open a file that wasn't mounted in Gramine's virtual filesystem).

Also, there are command-line tools for manipulating protected files (such as pf-crypt), which also have no notion of Gramine path (just host path).

[mkow]

Reviewed 17 of 27 files at r1, 1 of 3 files at r2, 7 of 10 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

OK, I updated that file to describe the current state better.

(As said elsewhere, I want to keep the name "protected files" for that library, but the library also talks about the feature as implemented in Gramine).

That's fine, it just had a lot of now-incorrect text (like being in common, but talking about SGX).

---

---

Documentation/manifest-syntax.rst line 655 at r4 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Also, there are command-line tools for manipulating protected files (such as pf-crypt), which also have no notion of Gramine path (just host path).

Hmm, ok, I see. Although it sounds weird here, because it sounds like we're verifying something on the untrusted side. But I don't have any better idea how to write this given the current implementation, so I'm not blocking.

---

Documentation/manifest-syntax.rst line 669 at r4 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Again, open() via different Gramine paths is possible, and our tests even verify that (without triggering the issue, because they don't keep the file open via both paths at the same time...) We only verify the host path.

Ok, maybe then "a single host path in Gramine" instead of "a single path in Gramine"?

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 221 at r5 (raw file):

}

int dump_pf_key(const pf_key_t* pf_key, char* buf, size_t buf_size) {

Could you add TODO: Deprecation started in v1.2, drop in 2 releases. here? This one is non-static, so we won't get a nice warning when we remove its usages.

---

LibOS/shim/test/regression/attestation.c line 148 at r5 (raw file):

 * TODO: remove this part of the test when these deprecated interfaces are removed. The new way of
 * setting keys (`/dev/attestation/keys`, `fs.insecure__keys`) is already tested in `keys.c`.
 */

Could you make it more similar toTODO: Deprecation started in v1.2, drop in 2 releases., so we won't miss this place when grepping?

[mkow]

Reviewable status: all 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

a discussion (no related file):
@pwmarcz: Please take a look at https://leeroy.cs.unc.edu/job/graphene-sgx-18.04/7613/testReport/junit/LibOS.shim.test.regression.test_libos/TC_30_Syscall/test_110_fcntl_lock/, it timed out.

---

[dimakuv]

Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

Documentation/manifest-syntax.rst line 655 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, ok, I see. Although it sounds weird here, because it sounds like we're verifying something on the untrusted side. But I don't have any better idea how to write this given the current implementation, so I'm not blocking.

Yes, Pawel said everything correctly. That's how the internal logic in Protected Files works -- it memorizes the host's filename.

[pwmarcz]

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

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

@pwmarcz: Please take a look at https://leeroy.cs.unc.edu/job/graphene-sgx-18.04/7613/testReport/junit/LibOS.shim.test.regression.test_libos/TC_30_Syscall/test_110_fcntl_lock/, it timed out.

I took a look, but I'm not sure what happens, and I cannot reproduce the problem. The test times out (20 s) on Jenkins. On my SGX-enabled machine, it takes about 6.4 seconds on master, about 5.7 seconds on this branch, and did not hang up in the ~40 runs that I did on this branch. So I don't expect the failure to be related to this PR... maybe some Jenkins workers are just slow?

(Times averaged over 10 runs. The long running time is because the test forks multiple times, creating new enclaves... and the speedup, I guess, is due to removing the protected files overhead in SGX PAL?)

---

---

Documentation/manifest-syntax.rst line 655 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, Pawel said everything correctly. That's how the internal logic in Protected Files works -- it memorizes the host's filename.

Yeah, I have no idea for a better explanation if it's going to be as short.

---

Documentation/manifest-syntax.rst line 669 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Ok, maybe then "a single host path in Gramine" instead of "a single path in Gramine"?

I reworded the explanation. (and removed the bit about host-level symlinks, because I realized that scenario is impossible)

---

LibOS/shim/src/fs/shim_fs_encrypted.c line 221 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you add TODO: Deprecation started in v1.2, drop in 2 releases. here? This one is non-static, so we won't get a nice warning when we remove its usages.

OK, I added a TODO to the header file.

---

LibOS/shim/test/regression/attestation.c line 148 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you make it more similar toTODO: Deprecation started in v1.2, drop in 2 releases., so we won't miss this place when grepping?

Done, although I expect this test to fail loudly then.

[mkow]

Reviewed 3 of 3 files at r6, 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

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I took a look, but I'm not sure what happens, and I cannot reproduce the problem. The test times out (20 s) on Jenkins. On my SGX-enabled machine, it takes about 6.4 seconds on master, about 5.7 seconds on this branch, and did not hang up in the ~40 runs that I did on this branch. So I don't expect the failure to be related to this PR... maybe some Jenkins workers are just slow?

(Times averaged over 10 runs. The long running time is because the test forks multiple times, creating new enclaves... and the speedup, I guess, is due to removing the protected files overhead in SGX PAL?)

Ok, maybe it's really just a slow machine...

---

[mkow]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)

[dimakuv]

Reviewed 1 of 1 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved