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).

This was originally developed as gramineproject/graphene#2641. Based on an offline discussion with @mkow, I simplified the code as much as possible (it overapproximates some things, but we don't care about a handful of envvars.)

Fixes gramineproject/graphene#2356

How to test this PR?

A new LibOS regression test is added + old tests should be sufficient.

---

This change is 

[pwmarcz]

Reviewed 6 of 6 files at r1, 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)

---

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

   loader.env.[ENVIRON] = { value = "[VALUE]" }
   or
   loader.env.[ENVIRON] = { passthrough = [true|false] }

Why do we even allow { passthrough = false }? As far as I can tell, it's either disallowed, or has no effect.

(And if we disallow passthrough = false, the "forbidden combination" can be detected directly in get_env_value_from_manifest.)

---

Pal/src/db_main.c, line 146 at r1 (raw file):

 * that its failure finishes the execution of the whole process right away. */
static int deep_copy_envs(bool propagate, const char** envp, const char*** out_envp) {
    if (!propagate) {

propagate=false essentially converts this function into one creating an empty environment, right?

It's very confusing, I'd prefer to have a separate function, even if it complicates the call sites:

// instead of:
return deep_copy_envs(propagate, envp, out_envp);

// write:
return propagate ? deep_copy_envs(envp, out_envp) : create_empty_envs(out_envp);

---

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

        const char** new_envp = malloc(sizeof(*new_envp));
        if (!new_envp)
			return -PAL_ERROR_NOMEM;

You have tabs instead of spaces here.

---

Pal/src/db_main.c, line 152 at r1 (raw file):

        new_envp[0] = NULL;

		*out_envp = new_envp;

You have tabs instead of spaces here.

---

Pal/src/db_main.c, line 178 at r1 (raw file):
Is that true, though? With propagate false, there is no deep copy of original environment, just a pass-through of selected variables.

And either way, I think we need to describe what propagate does, it wasn't obvious to me.

How about something like:

Build environment for Gramine process, based on original environment (from host or file) or manifest.
If propagate is true, copies all variables from orig_envp, except the ones overriden in manifest.
Otherwise, copies only the variables from orig_envp that the manifest specifies as passthrough.

static int build_envs(const char** orig_envp, bool propagate, const char*** out_envp);

The proposed name probably could be better, but my point is: it's not helpful to think about "augmenting" the original environment if you sometimes end up not using it at all.

---

Pal/src/db_main.c, line 219 at r1 (raw file):

        return -PAL_ERROR_NOMEM;

    /* For simplicity, allocate each env anew; this is suboptimal but happens only once. First

I think the last sentence applies to the second loop. How about:

size_t idx = 0;

/* First, go through original variables and copy the ones that we're going to use
 * (because of `propagate`, or passthrough specified for that variable in manifest). */
for (const char** orig_env = envp; *orig_env; orig_env++) {
    ...
}

/* Then, go through the manifest variables and add copy the ones with value provided. */
for (ssize_t i = 0; i < toml_envs_cnt; i++) {
    ...
}

And I don't think the "allocate each env anew" part deserves justification.

---

Pal/src/db_main.c, line 268 at r1 (raw file):

        if (propagate && !env_val && !passthrough) {
            /* this is not a hard-coded envvar and its passthrough == false */
            log_error("Forbidden combination: 'loader.insecure__use_host_env' /"

As said above, I think we should forbid this combination regardless of the value of propagate.

This error can seem pretty arbitrary for the user, and doesn't explain what to do instead.

I would add an explanation ("for security reasons, Gramine doesn't allow to selectively disable passthrough for variables, we recommend you specify all variables explicitly instead") or, if that's too long, a starting point like "see documentation for loader.env for details".

---

Pal/src/db_main.c, line 557 at r1 (raw file):

        INIT_FAIL(-ret, "Augmenting environment variables based on the manifest failed");

    if (initial_environments != environments)

Is it ever the case that initial_environments == final_environments?

---

Pal/src/db_main.c, line 558 at r1 (raw file):

    if (initial_environments != environments)
        free(initial_environments);

You need to also free the individual strings.

[dimakuv]

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

a discussion (no related file):
Replied with quick comments for now. Tomorrow I'll fix everything.

---

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Why do we even allow { passthrough = false }? As far as I can tell, it's either disallowed, or has no effect.

(And if we disallow passthrough = false, the "forbidden combination" can be detected directly in get_env_value_from_manifest.)

Do you mean to:

• In the code, fail immediately when we find passthrough = false?
• In the docs, don't even mention the possibility of false?

I can do both, yeah. It just felt weird to "ignore" the false value in the documentation...

---

Pal/src/db_main.c, line 178 at r1 (raw file):

With propagate false, there is no deep copy of original environment, just a pass-through of selected variables.

No, that's not correct. The new environment is allocated, and original envvars are copied via strdup. So it is a deep copy.

---

Pal/src/db_main.c, line 557 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Is it ever the case that initial_environments == final_environments?

No, it is not possible. final_environments is always a new deep copy of the initial_environments (or a completely new environment object, if there was no env_src_file and use_host_env = false).

---

Pal/src/db_main.c, line 558 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

You need to also free the individual strings.

No, because initial_environments in this case is created via load_cstring_array() which mallocs only one object (see the comment there).

[pwmarcz]

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

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Do you mean to:

• In the code, fail immediately when we find passthrough = false?
• In the docs, don't even mention the possibility of false?

I can do both, yeah. It just felt weird to "ignore" the false value in the documentation...

Yes, I mean both. As for justifying it: we could say false is the default, so { passthrough = false } (without value) is the same as not saying anything about a variable. If you specify a variable, it needs to either have a value, or be a passthrough.

(By that logic, I guess we could admit { value = xyz, passthrough = false } and make it mean the same as { value = xyz }... not sure if that helps, though, or makes it more complicated).

---

Pal/src/db_main.c, line 178 at r1 (raw file):

No, that's not correct. The new environment is allocated, and original envvars are copied via strdup. So it is a deep copy.

I agree that is what happens. I guess by "deep copy" I understood a copy of the whole structure, so the description was confusing to me.

I still think there should be a better explanation of what this function does and how propagate works.

---

Pal/src/db_main.c, line 558 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, because initial_environments in this case is created via load_cstring_array() which mallocs only one object (see the comment there).

I see the comment, but I also see load_cstring_array make two mallocs (one for buf and one for array). So it looks like you should free(initial_environments[0]) and then free(initial_environments)?

(And the comment for load_cstring_array() should probably make that more clear).

[dimakuv]

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

---

Documentation/manifest-syntax.rst, line 162 at r1 (raw file):
Done.

(By that logic, I guess we could admit { value = xyz, passthrough = false } and make it mean the same as { value = xyz }... not sure if that helps, though, or makes it more complicated).

To me, it nakes it more complicated. So didn't implement this additional corner case.

---

Pal/src/db_main.c, line 146 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

propagate=false essentially converts this function into one creating an empty environment, right?

It's very confusing, I'd prefer to have a separate function, even if it complicates the call sites:

// instead of:
return deep_copy_envs(propagate, envp, out_envp);

// write:
return propagate ? deep_copy_envs(envp, out_envp) : create_empty_envs(out_envp);

Done. Good suggestion.

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

You have tabs instead of spaces here.

Done. I wonder how this happened.

---

Pal/src/db_main.c, line 152 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

You have tabs instead of spaces here.

Done.

---

Pal/src/db_main.c, line 178 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

No, that's not correct. The new environment is allocated, and original envvars are copied via strdup. So it is a deep copy.

I agree that is what happens. I guess by "deep copy" I understood a copy of the whole structure, so the description was confusing to me.

I still think there should be a better explanation of what this function does and how propagate works.

Done. Thanks for the awesome suggestion on the comment and function name!

---

Pal/src/db_main.c, line 219 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I think the last sentence applies to the second loop. How about:

size_t idx = 0;

/* First, go through original variables and copy the ones that we're going to use
 * (because of `propagate`, or passthrough specified for that variable in manifest). */
for (const char** orig_env = envp; *orig_env; orig_env++) {
    ...
}

/* Then, go through the manifest variables and add copy the ones with value provided. */
for (ssize_t i = 0; i < toml_envs_cnt; i++) {
    ...
}

And I don't think the "allocate each env anew" part deserves justification.

Done. Again, thanks for amazing suggestions! Your skill in explaining code succinctly is impressive :)

---

Pal/src/db_main.c, line 268 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

As said above, I think we should forbid this combination regardless of the value of propagate.

This error can seem pretty arbitrary for the user, and doesn't explain what to do instead.

I would add an explanation ("for security reasons, Gramine doesn't allow to selectively disable passthrough for variables, we recommend you specify all variables explicitly instead") or, if that's too long, a starting point like "see documentation for loader.env for details".

Done. Your explanation was too long, so I cut it a bit.

---

Pal/src/db_main.c, line 557 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, it is not possible. final_environments is always a new deep copy of the initial_environments (or a completely new environment object, if there was no env_src_file and use_host_env = false).

Pawel, you created a blocking comment here. Is there something I need to fix?

---

Pal/src/db_main.c, line 558 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I see the comment, but I also see load_cstring_array make two mallocs (one for buf and one for array). So it looks like you should free(initial_environments[0]) and then free(initial_environments)?

(And the comment for load_cstring_array() should probably make that more clear).

Done. Oops, you are right. Yes, the comment in load_cstring_array() was confusing, fixed it.

[pwmarcz]

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

---

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

   ``insecure__use_host_env`` and then disallow some of them using ``passthrough
   = false``. However, this deny list approach is intentionally prohibited for
   security reasons. Graphene loudly fails if any ``passthrough = false``

I don't think this requires a lengthy explanation, but "for security reasons" is too little, IMO: it just suggests that the explanation exists elsewhere, so it can be frustrating to encounter e.g. if you just saw the error message and came here to read more about it.

Maybe: "this deny list approach is intentionally prohibited because it's inherently insecure / doesn't provide any real security"?

(Not blocking, though).

---

Pal/src/db_main.c, line 557 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Pawel, you created a blocking comment here. Is there something I need to fix?

Right, I misread this line as initial_environments = final_environments. Nothing to fix here.

---

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

    // TODO: Envs from file should be able to override ones from the manifest, but current
    // code makes this hard to implement.
    ret = build_envs(orig_environments, use_host_env || env_src_file, &final_environments);

I think this would read better with /*propagate=*/use_host_env || env_src_file, but I won't insist.

[dimakuv]

FYI: Jenkins-18.04 timed out on apps.LTP.sendfile09 (even though both tests PASSED), not related and looks like a real timeout

[pwmarcz]

Reviewed 2 of 2 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

[mkow]

Reviewed 3 of 6 files at r1, 1 of 3 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

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

Graphene loudly fails if ``passthrough = false`` manifest options are set.

If you do not specify a variable in the manifest, it has the same effect as ``{ passthrough = false }``.

Aren't these two contradictory? Can I use passthrough = false in the manifests or not?

---

Pal/src/db_main.c, line 74 at r4 (raw file):

and the corresponding `*out_val` or `*out_passthrough` is set

How is the caller supposed to know which one was set?

---

Pal/src/db_main.c, line 93 at r4 (raw file):

        }

        if (!out_val)

I'd either drop these checks or convert into an assert at the beginning.

---

Pal/src/db_main.c, line 116 at r4 (raw file):

!out_val

ditto

---

Pal/src/db_main.c, line 129 at r4 (raw file):

    /* at this point, it must be `loader.env.key = { passthrough = true|false }`*/
    if (!out_passthrough)

ditto

---

Pal/src/db_main.c, line 279 at r4 (raw file):

        char* final_env = alloc_concat3(toml_env_key, strlen(toml_env_key), "=", 1, env_val,
                                        strlen(env_val));

You can pass -1 instead of these strlens

---

Pal/src/db_main.c, line 506 at r4 (raw file):

    /* we don't modify original `environments` but deep-copy them into `final_environments`,
     * augmented based on `loader.env.key` manifest options */

I'd drop this comment, because why would you even consider modifying environments? The env we're building here is for the emulated app, not for Gramine emulation code.

---

Pal/src/db_main.c, line 522 at r4 (raw file):

All "'passthrough' environment variables in the manifest are ignored.

This is weird, what this sentence actually means? I.e. what were you trying to say?

---

Pal/src/db_main.c, line 545 at r4 (raw file):

    } else {
        /* Environment variables are taken from the host. */
        orig_environments = environments;

Hmm, but this means that propagation doesn't work with the "env from file"?

[dimakuv]

a discussion (no related file):
Jenkins failed because it is a smart-ass. It merged my PR with the latest master:

Commit message: "Merge 22f1b3a5d0788181c31bc288084cf5c70ec8c717 into 5c505d44ab6ffa6d303cde51b4ccf922c3eaab88"

And of course everything failed because the latest master uses "Gramine" everywhere and my manifest file still uses "Graphene". I hate this Jenkins auto-merge feature.

---

[dimakuv]

Reviewable status: 2 of 6 files reviewed, 9 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 and @pwmarcz)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Jenkins failed because it is a smart-ass. It merged my PR with the latest master:

Commit message: "Merge 22f1b3a5d0788181c31bc288084cf5c70ec8c717 into 5c505d44ab6ffa6d303cde51b4ccf922c3eaab88"

And of course everything failed because the latest master uses "Gramine" everywhere and my manifest file still uses "Graphene". I hate this Jenkins auto-merge feature.

Done. I had to squash and rebase to the latest master, sorry for this inconvinience.

---

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Graphene loudly fails if ``passthrough = false`` manifest options are set.

If you do not specify a variable in the manifest, it has the same effect as ``{ passthrough = false }``.

Aren't these two contradictory? Can I use passthrough = false in the manifests or not?

Ok, fair enough, bad wording. I removed the If you do not specify ... sentence -- it was misleading (I wanted to say "if you don't specify anything, then the behavior is no-passthrough").

---

Pal/src/db_main.c, line 74 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

and the corresponding `*out_val` or `*out_passthrough` is set

How is the caller supposed to know which one was set?

Done. I reworded the comment. Basically, the caller checks *out_val == NULL: if NULL, then it is a passthrough variable, otherwise it is a value variable.

---

Pal/src/db_main.c, line 93 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd either drop these checks or convert into an assert at the beginning.

Done. Moved them all on top (we currently always pass all arguments, and if this changes in the future, we'll re-shuffle the asserts).

---

Pal/src/db_main.c, line 116 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

!out_val

ditto

Done.

---

Pal/src/db_main.c, line 129 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

---

Pal/src/db_main.c, line 279 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

You can pass -1 instead of these strlens

Done. Didn't notice this trick, thanks.

---

Pal/src/db_main.c, line 506 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd drop this comment, because why would you even consider modifying environments? The env we're building here is for the emulated app, not for Gramine emulation code.

Done.

---

Pal/src/db_main.c, line 522 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

All "'passthrough' environment variables in the manifest are ignored.

This is weird, what this sentence actually means? I.e. what were you trying to say?

I meant to be explicit that loader.env.foo = { passthrough = true } will have no effect (they are consumed by loader.insecure__use_host_env).

I guess this is obvious, right? Let me just remove this sentence.

---

Pal/src/db_main.c, line 545 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, but this means that propagation doesn't work with the "env from file"?

Yes. "Env from file" approach means "the set of envvars is not taken from the host, it is taken from the file". So propagation of host envvars will not happen. But this looks the right way to do this, why would anyone want to mix "envs from file" and "envs from host"? This is also what our documentation seems to imply: loader.env_src_file allows to specify a URI to a file containing serialized environment.

Or maybe you're asking another question: "will we propagate only those envs-from-file that have { propagate = true }"? No, that's not the case. Notice the build_envs(.., /*propagate=*/use_host_env || env_src_file, ..) below. If we have the env-from-file approach, then propagate=true and all envs from file will be propagated regardless of { propagate = true }. (But envs from file may be overwritten by { value = "xxx" }, just like what the TODO comment says.)

[pwmarcz]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mkow)

[mkow]

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

a discussion (no related file):

And of course everything failed because the latest master uses "Gramine" everywhere and my manifest file still uses "Graphene". I hate this Jenkins auto-merge feature.

Everyone hates it :) But no one yet found a way to disable this "feature" of Jenkins.

---

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Ok, fair enough, bad wording. I removed the If you do not specify ... sentence -- it was misleading (I wanted to say "if you don't specify anything, then the behavior is no-passthrough").

Much better now, thanks!

---

Pal/src/db_main.c, line 522 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I meant to be explicit that loader.env.foo = { passthrough = true } will have no effect (they are consumed by loader.insecure__use_host_env).

I guess this is obvious, right? Let me just remove this sentence.

Yeah, this part of the meaning is obvious and the other part is probably wrong - passthrough = false are probably still errored out?
So yes, I prefer the new version without this sentence at all :)

---

Pal/src/db_main.c, line 545 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes. "Env from file" approach means "the set of envvars is not taken from the host, it is taken from the file". So propagation of host envvars will not happen. But this looks the right way to do this, why would anyone want to mix "envs from file" and "envs from host"? This is also what our documentation seems to imply: loader.env_src_file allows to specify a URI to a file containing serialized environment.

Or maybe you're asking another question: "will we propagate only those envs-from-file that have { propagate = true }"? No, that's not the case. Notice the build_envs(.., /*propagate=*/use_host_env || env_src_file, ..) below. If we have the env-from-file approach, then propagate=true and all envs from file will be propagated regardless of { propagate = true }. (But envs from file may be overwritten by { value = "xxx" }, just like what the TODO comment says.)

Ok, makes sense, although I previously though of "env-from-file" just as an extension to the env list in the manifest.

[mkow]

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