[Pal] Add loader.env.[ENVIRON] = {passthrough=true} manifest option

[dimakuv]

Description of the changes

It is now possible to define some envvars that are passed through from the host inside the Graphene instance (previously the only way to pass envvars was to set loader.insecure__use_host_env).

Fixes #2356.

How to test this PR?

A new LibOS regression test is added.

---

This change is 

[woju]

Reviewed 5 of 6 files at r1, all commit messages.
Reviewable status: 5 of 6 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

[mkow]

Reviewed 2 of 6 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all 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 @dimakuv)

---

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

   = false``. However, this deny list approach is intentionally prohibited.
   Graphene completely ignores any ``passthrough`` manifest options when it
   notices ``insecure__use_host_env``.

Shouldn't we fail loudly in this case then? If you do what you described here, then users will just not notice that the blacklist was ignored.

---

Pal/src/db_main.c, line 71 at r2 (raw file):

 *   - `loader.env.{key} = "val"`
 *   - `loader.env.{key} = { value = "val" }`
 *   - `loader.env.{key} = { passthrough = true|false }`, then `val_out = NULL`

not *val_out?

---

Pal/src/db_main.c, line 72 at r2 (raw file):

 *   - `loader.env.{key} = { value = "val" }`
 *   - `loader.env.{key} = { passthrough = true|false }`, then `val_out = NULL`
 *   - `loader.env.{key}` doesn't exist, then returns 0 and `val_out = NULL`

ditto

---

Pal/src/db_main.c, line 73 at r2 (raw file):

 *   - `loader.env.{key} = { passthrough = true|false }`, then `val_out = NULL`
 *   - `loader.env.{key}` doesn't exist, then returns 0 and `val_out = NULL`
 *

This table seems to be missing passthrough_out description altogether, is this intended or you just added it later and forgot to update? ;)

---

Pal/src/db_main.c, line 77 at r2 (raw file):

 */
static int get_env_value_from_manifest(toml_table_t* toml_envs, const char* key, char** val_out,
                                       bool* passthrough_out) {

should be out_ (i.e. prefix, not suffix; x2)

---

Pal/src/db_main.c, line 149 at r2 (raw file):

    if (toml_envs_cnt == 0) {
        /* no env entries found in the manifest */
        return 0;

Do I read correctly, that you forward all the variables in this case? I don't like the signature and semantics of this whole function, especially the in-out argument, I almost missed this.

---

Pal/src/db_main.c, line 181 at r2 (raw file):

        }

        /* at this point, we must passthrough envvar named `toml_env_key` from the host */

Please add a comment that this is O(host_env_cnt*passthrough_envs), but that shouldn't be a problem.

---

Pal/src/db_main.c, line 185 at r2 (raw file):

            char* orig_env_key_end = strchr(*orig_env, '=');

            *orig_env_key_end = '\0';

You're temporarily destroying env of the current process, I don't like it. Why not just use strstartswith and then additionally check for =?

---

Pal/src/db_main.c, line 241 at r2 (raw file):

            return -PAL_ERROR_INVAL;

        *orig_env_key_end = '\0';

ditto, here you even leave the env destroyed in case of an error

---

Pal/src/db_main.c, line 553 at r2 (raw file):

        }
    } else {
        ret = passthrough_envs_from_manifest(&environments);

We can't edit environments, it holds the current env for this very host process (it should actually be const, but for some reason isn't). Or maybe it actually doesn't edit it, but overwrites the pointer... No idea, this is too confusing, please refactor ;)

[dimakuv]

Reviewable status: all 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 @dimakuv and @mkow)

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't we fail loudly in this case then? If you do what you described here, then users will just not notice that the blacklist was ignored.

There was a reason why I chose to NOT fail when both insecure__use_host_env and env.blabla are specified. First, it is already possible to mix them together (then env.blabla = "foo" envs overwrite the insecure host envs) -- I definitely don't want to prohibit this legacy logic.

Second, it is very common to add insecure__use_host_env = true during debugging. Failing on a combination of insecure__use_host_env and env.blabla would mean that the debugging person needs to remove/comment out all env.blabla lines in the manifest, and then re-add/re-enable these lines again after debugging. I find it cumbersome.

What I can propose is: always fail on encountering env.blabla.passthrough = false. This setting is anyway meaningless and ignored by my current code. Does this sound reasonable?

[mkow]

Reviewable status: all 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 @dimakuv)

---

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

There was a reason why I chose to NOT fail when both insecure__use_host_env and env.blabla are specified. First, it is already possible to mix them together (then env.blabla = "foo" envs overwrite the insecure host envs) -- I definitely don't want to prohibit this legacy logic.

Wait, but that's a different case. I don't want to fail in this one, but fail if both insecure__use_host_env and env.blabla.passthrough = false were specified. Or to support it. But not silently ignore, this may lead to people deploying insecure configs.

You may now say that this makes .passthrough a tri-state option - true/false/not-specified, but it's not really this way, it's more like each env can be in one of the following states: (implicitly derived from which fields were set)

• hardcoded in the manifest/env src file
• passed through from the host
• explicitly forbidden to be passed through

[dimakuv]

Reviewable status: 4 of 6 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 @mkow)

---

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

Previously, mkow (Michał Kowalczyk) wrote…

There was a reason why I chose to NOT fail when both insecure__use_host_env and env.blabla are specified. First, it is already possible to mix them together (then env.blabla = "foo" envs overwrite the insecure host envs) -- I definitely don't want to prohibit this legacy logic.

Wait, but that's a different case. I don't want to fail in this one, but fail if both insecure__use_host_env and env.blabla.passthrough = false were specified. Or to support it. But not silently ignore, this may lead to people deploying insecure configs.

You may now say that this makes .passthrough a tri-state option - true/false/not-specified, but it's not really this way, it's more like each env can be in one of the following states: (implicitly derived from which fields were set)

• hardcoded in the manifest/env src file
• passed through from the host
• explicitly forbidden to be passed through

Done. Added check_passthrough_false_envs_in_manifest() to explicitly check for the combination of insecure__use_host_env and env.blabla.passthrough = false.

---

Pal/src/db_main.c, line 71 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

not *val_out?

Done.

---

Pal/src/db_main.c, line 72 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

---

Pal/src/db_main.c, line 73 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This table seems to be missing passthrough_out description altogether, is this intended or you just added it later and forgot to update? ;)

Done.

---

Pal/src/db_main.c, line 77 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

should be out_ (i.e. prefix, not suffix; x2)

Done.

---

Pal/src/db_main.c, line 149 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Do I read correctly, that you forward all the variables in this case? I don't like the signature and semantics of this whole function, especially the in-out argument, I almost missed this.

Done. This semantics was already like this in the old code (see old insert_envs_from_manifest), but I agree, it is time for refactoring.

---

Pal/src/db_main.c, line 181 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please add a comment that this is O(host_env_cnt*passthrough_envs), but that shouldn't be a problem.

Done.

---

Pal/src/db_main.c, line 185 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

You're temporarily destroying env of the current process, I don't like it. Why not just use strstartswith and then additionally check for =?

Done. Used alloc_substr because we still need a properly NULL-terminated string (here and in other places).

---

Pal/src/db_main.c, line 241 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto, here you even leave the env destroyed in case of an error

Done.

---

Pal/src/db_main.c, line 553 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

We can't edit environments, it holds the current env for this very host process (it should actually be const, but for some reason isn't). Or maybe it actually doesn't edit it, but overwrites the pointer... No idea, this is too confusing, please refactor ;)

Done. Now I deep-copy all envvars (even twice!), but this is only during init so who cares. Hopefully it's more readable now.

[mkow]

Reviewed 2 of 2 files at r3, all commit messages.
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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

Pal/src/db_main.c, line 185 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Used alloc_substr because we still need a properly NULL-terminated string (here and in other places).

Why do we need this? You deallocate it right afterwards.

---

Pal/src/db_main.c, line 73 at r3 (raw file):

 *   - `loader.env.{key} = { passthrough = true|false }`, then `*out_val = NULL` and
 *                                                             `*out_passthrough = true|false`
 *   - `loader.env.{key}` doesn't exist, then returns 0 and `*out_val = NULL`

What about *out_passthrough in this case? In the previous case we need to read *out_passthrough if *out_val = NULL, so when this one happens we'd read uninitialized memory? (note that we can't (and probably don't need) differentiate both from the caller).

---

Pal/src/db_main.c, line 138 at r3 (raw file):

    size_t idx = 0;
    for (const char** orig_env = envp; *orig_env; orig_env++) {
        new_envp[idx] = alloc_substr(*orig_env, strlen(*orig_env));

you can use strdup() instead

---

Pal/src/db_main.c, line 256 at r3 (raw file):

        /* at this point, we must passthrough envvar named `toml_env_key` from the host; this is
         * O(host_env_cnt * passthrough_envs), but happens only once during init so don't care */

so don't care -> so we don't care (sounds weird without this)

---

Pal/src/db_main.c, line 353 at r3 (raw file):

    for (const char** orig_env = envp; *orig_env; orig_env++) {
        char* orig_env_key_end = strchr(*orig_env, '=');
        assert(orig_env_key_end);

Can't this happen in runtime?

[dimakuv]

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

---

Pal/src/db_main.c, line 185 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why do we need this? You deallocate it right afterwards.

I do strcmp(env_key...) before free(env_key). I find it easier to allocate a properly NULL-terminated string and compare it using strcmp rather than looking at corner cases of a non-NULL terminated string.

---

Pal/src/db_main.c, line 73 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What about *out_passthrough in this case? In the previous case we need to read *out_passthrough if *out_val = NULL, so when this one happens we'd read uninitialized memory? (note that we can't (and probably don't need) differentiate both from the caller).

Done. I added a comment that *out_passthrough = false in this case (which is obvious from the code -- we set *out_passthrough = false in the very beginning). As you correctly mentioned, in the callers we don't differentiate between the cases 3 and 4 (both return 0 and *out_val = NULL), because the callers already handle these two cases in a different manner.

---

Pal/src/db_main.c, line 138 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

you can use strdup() instead

Done. I wanted to have alloc_substr() everywhere for uniformity, but ok.

---

Pal/src/db_main.c, line 256 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

so don't care -> so we don't care (sounds weird without this)

Done.

---

Pal/src/db_main.c, line 353 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Can't this happen in runtime?

No, it can't happen in runtime. Because about 20 lines above, we do the exact same for loop and have a proper if (!orig_env_key_end) check. So if the envvar doesn't have = in itself, this will be caught in that first for loop.

[mkow]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all 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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

Pal/src/db_main.c, line 185 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I do strcmp(env_key...) before free(env_key). I find it easier to allocate a properly NULL-terminated string and compare it using strcmp rather than looking at corner cases of a non-NULL terminated string.

But why not strncmp?

[dimakuv]

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

a discussion (no related file):
I will simplify this PR to do only one pass over envvars.

---

[dimakuv]

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

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I will simplify this PR to do only one pass over envvars.

[ Trying to publish again ]

---

[dimakuv]

Jenkins, retest Docs-18.04 please (just testing)

[dimakuv]

Jenkins, test Jenkins-18.04 please (just testing)

[dimakuv]

Recreated as gramineproject/gramine#56. Closing this one.