[Pal/Linux-SGX] Define SGX allowed/trusted/protected files as TOML arrays #2644

dimakuv · 2021-08-11T12:41:49Z

Description of the changes

This PR adds a new manifest syntax to define lists of SGX allowed, trusted, protected files. The previous syntax used TOML tables:

  sgx.trusted_files.file1 = "file:foo"
  sgx.trusted_files.file2 = "file:bar"

The new syntax uses TOML arrays:

  sgx.trusted_files = [ "file:foo", "file:bar" ]

The new syntax also allows to specify SHA256 hashes for a subset of trusted files (to skip hash generation during graphene-sgx-sign):

  [[sgx.trusted_files]]
  uri  = "file:trusted_testfile"
  sha256 = "c49a0aae384a14c8320f015ed5958d4402ba0726a31c4230cf772f76ff8aca2e"

The previous TOML-table syntax is still supported but deprecated. Graphene utility graphene-sgx-sign generates final SGX manifests using the new syntax, but graphene-sgx can still run old-syntax manifests.

All Graphene regression tests are updated to use the new syntax. But all examples still use the old syntax (to be fixed in next commits).

As a side effect, the TOML C library (tomlc99) is updated to the latest version -- it supports mixed TOML arrays.

Fixes #2593.

How to test this PR?

All tests should pass. Since I updated Graphene tests to the new syntax, and left the examples with the old syntax, both syntaxes will be tested in CI and both must succeed.

This change is

woju

Reviewed 22 of 29 files at r1.
Reviewable status: 21 of 29 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) (waiting on @dimakuv and @woju)

Pal/src/host/Linux-SGX/enclave_framework.c, line 792 at r1 (raw file):

        }

        toml_raw_t toml_trusted_hash_raw = toml_raw_in(toml_trusted_file, "hash");

Could we name this "sha256" instead of "hash"? 1) Explicit is better than implicit, 2) we may want to change the algorithm in the future (cf. SHA1 in git).

python/graphenelibos/sgx_sign.py, line 206 at r1 (raw file):

            if hash_ is None:
                hash_ = get_hash(target).hex()
            targets[idx] = uri, target, hash_

Are you sure this works? You're modifying the collection, over which you're iterating (via enumerate(targets) above), and while this sometimes works (eg. with lists), sometimes this breaks (notably hash-based structures, like dicts and sets). If you're not sure, I'd suggest to duplicate either collection (like list(enumerate(...))).

woju

Reviewable status: 21 of 29 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: Intel) (waiting on @dimakuv and @woju)

a discussion (no related file):
Also, please fix documentation, at least Documentation/manifest-syntax.rst and Pal/src/host/Linux-SGX/protected-files/README.rst.

pwmarcz

Reviewed 27 of 29 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

-- commits, line 28 at r2 ([raw file](https://github.com/oscarlab/graphene/blob/12f7b3d5e8ee057c725b7c4e5df388ace2374c0d/-- commits#L28)):
Can this be a separate commit, before the main one?

common/src/toml.patch, line 26 at r2 (raw file):

 			if (!arr) return -1;
 		}
-		if (arr->kind != 't')

What's happening here? Does that mean tomlc99 doesn't support mixed arrays in some cases, and you're patching it by removing the "array mismatch" error?

(Probably worth explaining in the commit message at least).

Pal/src/host/Linux-SGX/enclave_framework.c, line 781 at r2 (raw file):

    for (ssize_t i = 0; i < toml_trusted_files_cnt; i++) {
        /* read `sgx.trusted_file = {uri = "file:foo", hash = "deadbeef"}` entry from manifest */
        toml_table_t* toml_trusted_file = toml_table_at(toml_trusted_files, i);

What happens if the entry is not a TOML table (e.g. trusted_files = [ "foo", "bar" ]), do we get a NULL here? Can you handle it?

Pal/src/host/Linux-SGX/enclave_framework.c, line 784 at r2 (raw file):

        toml_raw_t toml_trusted_uri_raw = toml_raw_in(toml_trusted_file, "uri");
        assert(toml_trusted_uri_raw);

This can be null if there's no uri key, right? Is it a good idea to use asserts instead of checking the value and returning an error?

I assume you're doing this because the manifest has been produced and validated by our (Python?) tools. But if so, it would still be better to return an error instead of debug-mode assert...

Pal/src/host/Linux-SGX/enclave_pf.c, line 518 at r2 (raw file):

        ret = register_protected_files_from_toml_array();
        if (ret == -PAL_ERROR_NOTDEFINED) {
            /* no allowed files were found, perfectly valid case */

Using an error code (-PAL_ERROR_NOTDEFINED) as a valid case looks a bit awkward. How about making these functions return the number of files parsed? Then you can call the other one if the number is 0.

python/graphenelibos/sgx_sign.py, line 167 at r2 (raw file):

def append_trusted_dir_or_file(targets, val, check_exist):
    try:

This looks convoluted, the except: branch does no check that val is actually a string. I think it's much better to check the type directly:

if isinstance(val, dict):
    uri = val['uri']
    hash = val['hash']
elif isinstance(val, str):
    uri = val
    hash = None
else:
    raise ValueError(f'unknown trusted file format: {val!r}')

pwmarcz

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @woju)

python/graphenelibos/sgx_sign.py, line 206 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Are you sure this works? You're modifying the collection, over which you're iterating (via enumerate(targets) above), and while this sometimes works (eg. with lists), sometimes this breaks (notably hash-based structures, like dicts and sets). If you're not sure, I'd suggest to duplicate either collection (like list(enumerate(...))).

If you want to modify it in place, you can use the index (also, modify only if it needs changing):

for i in range(len(targets)):
     uri, target, hash_ = targets[i]
    if hash_ is None:
        hash_ = get_hash(target).hex()
        targets[i] = uri, target, hash_

If you don't mind copying the array, this looks more straightforward:

hashed_targets = []
for (uri, target, hash_) in targets:
    if hash_ is None:
        hash_ = get_hash(target).hex()
    hashed_targets.append((uri, target, hash_))

dimakuv

Reviewable status: 23 of 31 files reviewed, 10 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 @pwmarcz and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Also, please fix documentation, at least Documentation/manifest-syntax.rst and Pal/src/host/Linux-SGX/protected-files/README.rst.

Done.

-- commits, line 28 at r2:

Previously, pwmarcz (Paweł Marczewski) wrote…

Can this be a separate commit, before the main one?

Done

common/src/toml.patch, line 26 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

What's happening here? Does that mean tomlc99 doesn't support mixed arrays in some cases, and you're patching it by removing the "array mismatch" error?

(Probably worth explaining in the commit message at least).

Done. I removed this hack. This was a leftover from my hacking, thanks for noticing.

common/src/toml.patch, line 91 at r3 (raw file):

 	*ret = strtoll(buf, &endp, base);
-	return (errno || *endp) ? -1 : 0;
+    return (*endp) ? -1 : 0;

Meh... Could we ignore this blooper? I don't want to recreate the patch file.

Pal/src/host/Linux-SGX/enclave_framework.c, line 792 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Could we name this "sha256" instead of "hash"? 1) Explicit is better than implicit, 2) we may want to change the algorithm in the future (cf. SHA1 in git).

Done.

Note that I only changed it in this new TOML syntax and in this function: the old TOML syntax uses sgx.trusted_checksum (and is kept for legacy reasons) and the other C functions use hash words (because it is not important how we name it locally in our C code and because it would require a lot of unrelated changes).

Pal/src/host/Linux-SGX/enclave_framework.c, line 781 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

What happens if the entry is not a TOML table (e.g. trusted_files = [ "foo", "bar" ]), do we get a NULL here? Can you handle it?

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 784 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This can be null if there's no uri key, right? Is it a good idea to use asserts instead of checking the value and returning an error?

I assume you're doing this because the manifest has been produced and validated by our (Python?) tools. But if so, it would still be better to return an error instead of debug-mode assert...

Done.

No, I did this because I was in vacation mode already that day...

Pal/src/host/Linux-SGX/enclave_pf.c, line 518 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Using an error code (-PAL_ERROR_NOTDEFINED) as a valid case looks a bit awkward. How about making these functions return the number of files parsed? Then you can call the other one if the number is 0.

Done.

Actually did some more refactoring to make allowed / trusted / protected files logic similar and added more error checking.

python/graphenelibos/sgx_sign.py, line 206 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

If you want to modify it in place, you can use the index (also, modify only if it needs changing):

for i in range(len(targets)):
     uri, target, hash_ = targets[i]
    if hash_ is None:
        hash_ = get_hash(target).hex()
        targets[i] = uri, target, hash_

If you don't mind copying the array, this looks more straightforward:

hashed_targets = []
for (uri, target, hash_) in targets:
    if hash_ is None:
        hash_ = get_hash(target).hex()
    hashed_targets.append((uri, target, hash_))

Done.

python/graphenelibos/sgx_sign.py, line 167 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This looks convoluted, the except: branch does no check that val is actually a string. I think it's much better to check the type directly:
if isinstance(val, dict):
    uri = val['uri']
    hash = val['hash']
elif isinstance(val, str):
    uri = val
    hash = None
else:
    raise ValueError(f'unknown trusted file format: {val!r}')

Done.

dimakuv

Reviewable status: 23 of 31 files reviewed, 10 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 @pwmarcz and @woju)

python/graphenelibos/sgx_sign.py, line 206 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Done. Used the approach with duplication (new collection). The first approach made Pylint unhappy:

C:204, 8: Consider using enumerate instead of iterating with range and len (consider-using-enumerate)

woju

Reviewed 4 of 8 files at r3, 1 of 1 files at r4.
Reviewable status: 28 of 31 files reviewed, 7 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 @pwmarcz)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Thanks.

Pal/src/host/Linux-SGX/enclave_framework.c, line 792 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Note that I only changed it in this new TOML syntax and in this function: the old TOML syntax uses sgx.trusted_checksum (and is kept for legacy reasons) and the other C functions use hash words (because it is not important how we name it locally in our C code and because it would require a lot of unrelated changes).

Yes, certainly. Thanks.

python/graphenelibos/sgx_sign.py, line 206 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Used the approach with duplication (new collection). The first approach made Pylint unhappy:
C:204, 8: Consider using enumerate instead of iterating with range and len (consider-using-enumerate)

Seems bogus, if you'd like, add to ignored in .pylintrc. Resolving here.

mkow

Reviewed 21 of 29 files at r1, 2 of 2 files at r2, 7 of 8 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 30 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 and @pwmarcz)

common/src/toml.patch, line 91 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Meh... Could we ignore this blooper? I don't want to recreate the patch file.

Can't you edit this line manually in the .patch file? ;)

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

::

    sgx.allowed_files = [ "[URI]", "[URI]" ]

I'd use a nicer formatting here, as people will most likely just copy paste it, plus they'll have many more files. E.g.:

sgx.allowed_files = [
  "[URI]",
  "[URI]",
]

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

    sgx.allowed_files = [ "[URI]", "[URI]" ]
    # Or, deprecated TOML-table syntax:
    # sgx.allowed_files.[identifier] = "[URI]"

I wouldn't list the old syntax at all and just purge it from any documentation + warn if used. And then stop supporting it at some point.

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

::

    sgx.trusted_files = [ "[URI]", "[URI]" ]  # entries can be strings

ditto

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

    sgx.trusted_files = [ "[URI]", "[URI]" ]  # entries can be strings

    [[sgx.trusted_files]]                     # entries can be tables

entries can be tables -> entries can also be tables

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

automatically generate hashes of these files and add them to the SGX-specific
manifest (``.manifest.sgx``). The manifest writer may also specify the hash for
a file using the TOML-table syntax, in the field ``sha256``.

Please mention explicitly that hashing this file will be skipped by graphene-sgx-sign tool and this value will be used instead.

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

    sgx.protected_files_key = "[16-byte hex value]"

    sgx.protected_files = [ "[URI]", "[URI]" ]

ditto

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

    sgx.protected_files = [ "[URI]", "[URI]" ]
    # Or, deprecated TOML-table syntax:
    # sgx.protected_files.[identifier] = "[URI]"

ditto

LibOS/shim/test/fs/manifest.template, line 46 at r4 (raw file):

sgx.protected_files = [
  "file:tmp/pf_input",
  "file:tmp/pf_output"

I prefer putting the comma after the last element if it's listed one-per-line. The diff is then nicer if you add/reshuffle the lines, and you can also move them easier when experimenting with code (no need to fix the missing comma).
ditto for all other places

LibOS/shim/test/regression/rename.manifest.template, line 22 at r4 (raw file):

]

sgx.allowed_files = [ "file:tmp/" ]

In other manifests you listed allowed files first, could you do the same here?

Pal/regression/manifest.template, line 17 at r4 (raw file):

]

sgx.allowed_files = [

ditto

Pal/src/host/Linux-SGX/enclave_framework.c, line 560 at r4 (raw file):

    int ret;

    if (checksum_str && strlen(checksum_str) < sizeof(sgx_file_hash_t) * 2) {

not !=?

Pal/src/host/Linux-SGX/enclave_framework.c, line 752 at r4 (raw file):

              "= [\"file1\", ..]'.");

    ret = (int)toml_trusted_files_cnt;

could we return this via argument? also, is it really used?

Pal/src/host/Linux-SGX/enclave_framework.c, line 777 at r4 (raw file):

        return 0;

    char* toml_trusted_uri_str  = NULL;

double space

Pal/src/host/Linux-SGX/enclave_framework.c, line 824 at r4 (raw file):

        free(toml_trusted_uri_str);
        free(toml_trusted_sha256_str);
        toml_trusted_uri_str  = NULL;

doublespace

Pal/src/host/Linux-SGX/enclave_framework.c, line 828 at r4 (raw file):

    }

    ret = (int)toml_trusted_files_cnt;

ditto (ret via arg)

Pal/src/host/Linux-SGX/enclave_framework.c, line 884 at r4 (raw file):

              "= [\"file1\", ..]'.");

    ret = (int)toml_allowed_files_cnt;

ditto

Pal/src/host/Linux-SGX/enclave_framework.c, line 932 at r4 (raw file):

    }

    ret = (int)toml_allowed_files_cnt;

ditto

Pal/src/host/Linux-SGX/enclave_framework.c, line 953 at r4 (raw file):

    }

    log_debug("Found %d trusted files in the manifest", ret);

is this actually useful for anything?

Pal/src/host/Linux-SGX/enclave_framework.c, line 972 at r4 (raw file):

    }

    log_debug("Found %d allowed files in the manifest", ret);

ditto

Pal/src/host/Linux-SGX/enclave_pf.c, line 518 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Actually did some more refactoring to make allowed / trusted / protected files logic similar and added more error checking.

OTOH: We try to not mix values and error codes in return values and I think we got rid of a lot of such places recently. But I agree that a magic error code is hacky, so maybe just return by an arg?

Pal/src/host/Linux-SGX/enclave_pf.c, line 480 at r4 (raw file):

              "= [\"file1\", ..]'.");

    ret = (int)toml_pfs_cnt;

ditto

Pal/src/host/Linux-SGX/enclave_pf.c, line 533 at r4 (raw file):

    }

    ret = (int)toml_pfs_cnt;

ditto

python/graphenelibos/sgx_sign.py, line 170 at r4 (raw file):

        # trusted file is specified as TOML table `{uri = "file:foo", sha256 = "deadbeef"}`
        uri_ = val['uri']
        hash_ = val['sha256']

Won't this throw if only the URI was provided? Is this a valid case actually, or maybe we don't support it? If not, why?

python/graphenelibos/sgx_sign.py, line 178 at r4 (raw file):

        raise ValueError(f'Unknown trusted file format: {val!r}')

    path = Path(resolve_uri(uri_, check_exist))

Shouldn't we skip checking the existence if the hash was manually provided? Also, could you test this in LibOS regression? (that we don't require this file to be present when graphene-sgx-sign runs)

dimakuv

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

common/src/toml.patch, line 91 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Can't you edit this line manually in the .patch file? ;)

Done. Technically, the hash should have changed but who cares.

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

Previously, mkow (Michał Kowalczyk) wrote…

I'd use a nicer formatting here, as people will most likely just copy paste it, plus they'll have many more files. E.g.:
sgx.allowed_files = [
  "[URI]",
  "[URI]",
]

Done.

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

Previously, mkow (Michał Kowalczyk) wrote…

I wouldn't list the old syntax at all and just purge it from any documentation + warn if used. And then stop supporting it at some point.

Done.

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

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

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

Previously, mkow (Michał Kowalczyk) wrote…

entries can be tables -> entries can also be tables

Done.

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

Previously, mkow (Michał Kowalczyk) wrote…

Please mention explicitly that hashing this file will be skipped by graphene-sgx-sign tool and this value will be used instead.

Done.

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

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

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

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

LibOS/shim/test/fs/manifest.template, line 46 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I prefer putting the comma after the last element if it's listed one-per-line. The diff is then nicer if you add/reshuffle the lines, and you can also move them easier when experimenting with code (no need to fix the missing comma).
ditto for all other places

Done.

LibOS/shim/test/regression/rename.manifest.template, line 22 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

In other manifests you listed allowed files first, could you do the same here?

Done.

Pal/regression/manifest.template, line 17 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 560 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

not !=?

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 752 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

could we return this via argument? also, is it really used?

Done. Actually, there is no sense to use either TOML-table syntax or TOML-array syntax. I now use both, and thus the return value is irrelevant.

Pal/src/host/Linux-SGX/enclave_framework.c, line 777 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

double space

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 824 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

doublespace

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 828 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (ret via arg)

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 884 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 932 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

Pal/src/host/Linux-SGX/enclave_framework.c, line 953 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

is this actually useful for anything?

Done. No, it was not, removed.

Pal/src/host/Linux-SGX/enclave_framework.c, line 972 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

Pal/src/host/Linux-SGX/enclave_pf.c, line 518 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

OTOH: We try to not mix values and error codes in return values and I think we got rid of a lot of such places recently. But I agree that a magic error code is hacky, so maybe just return by an arg?

Done.

Pal/src/host/Linux-SGX/enclave_pf.c, line 480 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

Pal/src/host/Linux-SGX/enclave_pf.c, line 533 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

python/graphenelibos/sgx_sign.py, line 170 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Won't this throw if only the URI was provided? Is this a valid case actually, or maybe we don't support it? If not, why?

Done. Good catch...

python/graphenelibos/sgx_sign.py, line 178 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't we skip checking the existence if the hash was manually provided? Also, could you test this in LibOS regression? (that we don't require this file to be present when graphene-sgx-sign runs)

Done. Good catch. Changed the tests logic to create trusted_testfile on the fly, before running the corresponding tests.

mkow

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

common/src/toml.patch, line 91 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Technically, the hash should have changed but who cares.

Oh, true :) Whatever.

woju

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

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

I disagree, we shoudn't just remove the documentation of any supported schema. If you don't want it here, please add "Deprecated schema" (or sth like that) section somewhere near the end.

mkow

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

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

Previously, woju (Wojtek Porczyk) wrote…

I disagree, we shoudn't just remove the documentation of any supported schema. If you don't want it here, please add "Deprecated schema" (or sth like that) section somewhere near the end.

What's the point of keeping it? It only makes the docs longer and I don't see any case where someone should actually read about it.
It will cease to be supported soon.

pwmarcz

Reviewed 1 of 8 files at r3, 32 of 32 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @woju)

common/src/toml.patch, line 91 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Oh, true :) Whatever.

+1 for not caring too much about the hashes, editing the patches manually is too convenient (Emacs even adjusts the line numbers and counts if necessary :) ).

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

Previously, mkow (Michał Kowalczyk) wrote…

What's the point of keeping it? It only makes the docs longer and I don't see any case where someone should actually read about it.
It will cease to be supported soon.

Release notes?

mkow

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @woju)

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Release notes?

@pwmarcz: You mean adding this only to release notes? This is something which will definitely go there, I think this part is out of question :) (we always have "breaking changes" section)

woju

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

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

Previously, mkow (Michał Kowalczyk) wrote…

@pwmarcz: You mean adding this only to release notes? This is something which will definitely go there, I think this part is out of question :) (we always have "breaking changes" section)

This is orthogonal. Yes, this obviously should be mentioned in release notes, but I think as long we support this, we should have this documented. We can remove this when we'll stop supporting it.

mkow

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @woju)

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

Previously, woju (Wojtek Porczyk) wrote…

This is orthogonal. Yes, this obviously should be mentioned in release notes, but I think as long we support this, we should have this documented. We can remove this when we'll stop supporting it.

IMO it's just taking space in the docs and no one should ever need to use it anymore. So, I vote for removing it.

dimakuv

Reviewable status: 34 of 35 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 @mkow, @pwmarcz, and @woju)

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

Previously, mkow (Michał Kowalczyk) wrote…

IMO it's just taking space in the docs and no one should ever need to use it anymore. So, I vote for removing it.

Done. Added the Deprecated syntax section.

mkow

Reviewed all commit messages.
Reviewable status: 34 of 35 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv, @mkow, @pwmarcz, and @woju)

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Added the Deprecated syntax section.

For the record: we did a small vote on this and an additional section won.

Documentation/manifest-syntax.rst, line 717 at r6 (raw file):

listed below, with short descriptions.

Executable (removed)

What's the point of including all of these? They don't work anymore and no one is using them.

dimakuv

Reviewable status: 34 of 35 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow, @pwmarcz, and @woju)

Documentation/manifest-syntax.rst, line 717 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What's the point of including all of these? They don't work anymore and no one is using them.

Before I fixup: do you want me to remove all the (removed) manifest options?

What about Program Break Size (renamed)? Remove or keep?

mkow

Reviewable status: 34 of 35 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow, @pwmarcz, and @woju)

Documentation/manifest-syntax.rst, line 717 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Before I fixup: do you want me to remove all the (removed) manifest options?

What about Program Break Size (renamed)? Remove or keep?

I vote for removing everything which doesn't currently work (and I think that was @woju's original proposal, to document only things that still work).

woju

Reviewable status: 34 of 35 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv, @mkow, and @pwmarcz)

Documentation/manifest-syntax.rst, line 698 at r6 (raw file):

Deprecated syntax

This is "schema", not syntax. Syntax is TOML, we define schema inside.

Documentation/manifest-syntax.rst, line 717 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I vote for removing everything which doesn't currently work (and I think that was @woju's original proposal, to document only things that still work).

Right, we shouldn't document things that don't actually work. That's really for release notes and git log only.

mkow

Reviewed all commit messages.
Reviewable status: 34 of 35 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 (waiting on @dimakuv, @mkow, and @pwmarcz)

Documentation/manifest-syntax.rst, line 702 at r7 (raw file):

Before October 2020, manifests were not written in the TOML syntax but instead
in an ad-hoc INI file-like syntax. Some of the differences include:

What about this section? It also lists things which are long-gone, I see no point in documenting them.

pwmarcz

Reviewed all commit messages.
Reviewable status: 34 of 35 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 (waiting on @dimakuv, @mkow, and @pwmarcz)

Documentation/manifest-syntax.rst, line 717 at r6 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Right, we shouldn't document things that don't actually work. That's really for release notes and git log only.

+1, let's just document what works (even if it's about to be removed soon).

mkow

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

woju

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

dimakuv

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

Documentation/manifest-syntax.rst, line 698 at r6 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This is "schema", not syntax. Syntax is TOML, we define schema inside.

OK. Here I went with options and in the text about files I used schema.

Documentation/manifest-syntax.rst, line 717 at r6 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

+1, let's just document what works (even if it's about to be removed soon).

Done. Everything removed.

Documentation/manifest-syntax.rst, line 702 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What about this section? It also lists things which are long-gone, I see no point in documenting them.

Done. Removed.

Mixed arrays were recently added to the TOML language specification, and they allow to specify `sgx.trusted_files` as both strings and tables: sgx.trusted_files = [ "file:first_file", { uri = "file:second_file", sha256 = "abcdef..." } ] Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

…rays This commit adds a new manifest syntax to define lists of SGX allowed, trusted, protected files. The previous syntax used TOML tables: sgx.trusted_files.file1 = "file:foo" sgx.trusted_files.file2 = "file:bar" The new syntax uses TOML arrays: sgx.trusted_files = [ "file:foo", "file:bar" ] The new syntax also allows to specify SHA256 hashes for a subset of trusted files (to skip hash generation during `graphene-sgx-sign`): [[sgx.trusted_files]] uri = "file:trusted_testfile" sha256 = "c49a0aae384a14c8320f015ed5958d4402ba0726a31c4230cf772f76ff8aca2e" The previous TOML-table syntax is still supported but deprecated. Graphene utility `graphene-sgx-sign` generates final SGX manifests using the new syntax, but `graphene-sgx` can still run old-syntax manifests. All Graphene regression tests are updated to use the new syntax. But all examples still use the old syntax; to be fixed in next commits. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

mkow

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

woju

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

dimakuv · 2021-09-01T10:57:58Z

Jenkins, retest Jenkins-Debug-20.04 please (test_libos.TC_50_GDB.test_000_gdb_backtrace failed with AssertionError: timeout (10 s) expired)

dimakuv force-pushed the dimakuv/sgx-files-toml-arrays-and-more branch 2 times, most recently from 00a2b6b to 12f7b3d Compare August 11, 2021 14:20

woju suggested changes Aug 11, 2021

View reviewed changes

woju mentioned this pull request Aug 11, 2021

Redesign manifest #2076

Open

woju suggested changes Aug 11, 2021

View reviewed changes

pwmarcz suggested changes Aug 12, 2021

View reviewed changes

pwmarcz reviewed Aug 12, 2021

View reviewed changes

dimakuv force-pushed the dimakuv/sgx-files-toml-arrays-and-more branch from 12f7b3d to d4eb01e Compare August 30, 2021 14:01

dimakuv commented Aug 30, 2021

View reviewed changes

woju reviewed Aug 30, 2021

View reviewed changes

mkow suggested changes Aug 30, 2021

View reviewed changes

dimakuv commented Aug 31, 2021

View reviewed changes

mkow previously approved these changes Aug 31, 2021

View reviewed changes

woju suggested changes Aug 31, 2021

View reviewed changes

mkow reviewed Aug 31, 2021

View reviewed changes

pwmarcz previously approved these changes Aug 31, 2021

View reviewed changes

mkow reviewed Aug 31, 2021

View reviewed changes

woju suggested changes Aug 31, 2021

View reviewed changes

mkow reviewed Aug 31, 2021

View reviewed changes

dimakuv dismissed stale reviews from pwmarcz and mkow via f05ab35 August 31, 2021 16:01

dimakuv commented Aug 31, 2021

View reviewed changes

mkow suggested changes Aug 31, 2021

View reviewed changes

dimakuv commented Aug 31, 2021

View reviewed changes

mkow reviewed Aug 31, 2021

View reviewed changes

woju suggested changes Aug 31, 2021

View reviewed changes

mkow reviewed Aug 31, 2021

View reviewed changes

pwmarcz reviewed Aug 31, 2021

View reviewed changes

mkow previously approved these changes Aug 31, 2021

View reviewed changes

woju previously approved these changes Aug 31, 2021

View reviewed changes

dimakuv commented Aug 31, 2021

View reviewed changes

dimakuv dismissed stale reviews from woju and mkow via 91cb0be August 31, 2021 20:36

dimakuv force-pushed the dimakuv/sgx-files-toml-arrays-and-more branch from c035374 to 91cb0be Compare August 31, 2021 20:36

Dmitrii Kuvaiskii added 2 commits August 31, 2021 13:36

[Docs] Add "Deprecated options" section

1c0aad7

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

dimakuv force-pushed the dimakuv/sgx-files-toml-arrays-and-more branch from 91cb0be to 1c0aad7 Compare August 31, 2021 20:38

mkow approved these changes Aug 31, 2021

View reviewed changes

woju approved these changes Sep 1, 2021

View reviewed changes

dimakuv merged commit 1c0aad7 into master Sep 1, 2021

dimakuv deleted the dimakuv/sgx-files-toml-arrays-and-more branch September 1, 2021 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pal/Linux-SGX] Define SGX allowed/trusted/protected files as TOML arrays #2644

[Pal/Linux-SGX] Define SGX allowed/trusted/protected files as TOML arrays #2644

dimakuv commented Aug 11, 2021 •

edited

Loading

woju left a comment

woju left a comment

pwmarcz left a comment

pwmarcz left a comment

dimakuv left a comment

dimakuv left a comment

woju left a comment

mkow left a comment

dimakuv left a comment

mkow left a comment

woju left a comment

mkow left a comment

pwmarcz left a comment

mkow left a comment

woju left a comment

mkow left a comment

dimakuv left a comment

mkow left a comment

dimakuv left a comment

mkow left a comment

woju left a comment

mkow left a comment

pwmarcz left a comment

mkow left a comment

woju left a comment

dimakuv left a comment

mkow left a comment

woju left a comment

dimakuv commented Sep 1, 2021

[Pal/Linux-SGX] Define SGX allowed/trusted/protected files as TOML arrays #2644

[Pal/Linux-SGX] Define SGX allowed/trusted/protected files as TOML arrays #2644

Conversation

dimakuv commented Aug 11, 2021 • edited Loading

Description of the changes

How to test this PR?

woju left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

dimakuv commented Sep 1, 2021

dimakuv commented Aug 11, 2021 •

edited

Loading