Support Docker images that have a non-root user

[veenasai2]

Description of the changes

Currently, gsc does not support building app images that has a non-root user.

The gsc build step fails at line https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L11 , with the error as shown below

E: List directory /var/lib/apt/lists/partial is missing. - Acquire (2: No such file or directory)

This error occurs because distro-specific packages cannot be installed as non-root user.

If one bypass this error by putting "USER root" at https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L10 , then the unsigned image and the final signed gsc image will have user as "root".

This PR allows to create gsc images for app images with non-root user. Also, the original "user" of the app image will be restored back in the final signed gsc image.

Fixes #30.

How to test this PR?

Step 1: docker pull <non-root-user-app-image> //example of non-root user image bitnami pytorch
Step 2: docker inspect non-root-user-app-image // here you can check the original user
Step 3: perform gsc build and gsc sign on the non-root-user-app-image
Step 4: docker inspect gsc-non-root-user-app-image // Verify again, the image will have the original user
Step 5: docker run --device=/dev/sgx_enclave -it gsc-non-root-user-app-image //success

Step 6: gsc build and sign should pass for root user images as well.

Note: If the app image has an empty user field like user : "" , then the final gsc image will be created with root user.

---

This change is 

[aneessahib]

fixes issue #60

[dimakuv]

@aneessahib Did you mean #30? Looks like a typo.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @veenasai2)

---

-- commits line 2 at r1:
Rephrase: Support Docker images that have a non-root user

We can fix this during final rebase.

---

gsc.py line 177 at r1 (raw file):

    env.globals.update({'app_image': original_image_name})
    original_image_user = extract_user_from_image_config(original_image.attrs['Config'])
    env.globals.update({'app_user': original_image_user})

Could you please change the new function to be similar as already-existing functions like extract_binary_cmd_from_image_config()?

In particular, pass the env variable into the new function and perform env.globals.update() in the function itself.

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

# In the signed graminized image, the "user" will be restored.
ENV app_user={{app_user}}
USER root

Why so complicated? Why can't you temporarily switch to the root user (like you do now) and then -- immediately after block install -- switch back to normal user?

I mean, I propose this:

# temporarily switch to the root user to install packages
USER root

# Install distro-specific packages to run Gramine (e.g., python3, protobuf, toml, etc.)
{% block install %}{% endblock %}

# switch back to original app-image user
USER {{app_user}}

See also https://stackoverflow.com/a/63643361/6325371

In this case, you won't need any other modifications. In particular, you won't need /gramine/app_files changes.

Or did I miss something?

[aneessahib]

That's right, sorry.

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @veenasai2)

[veenasai2]

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

---

-- commits line 2 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Rephrase: Support Docker images that have a non-root user

We can fix this during final rebase.

Sure, thanks.

---

gsc.py line 177 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you please change the new function to be similar as already-existing functions like extract_binary_cmd_from_image_config()?

In particular, pass the env variable into the new function and perform env.globals.update() in the function itself.

done, thanks.

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why so complicated? Why can't you temporarily switch to the root user (like you do now) and then -- immediately after block install -- switch back to normal user?

I mean, I propose this:

# temporarily switch to the root user to install packages
USER root

# Install distro-specific packages to run Gramine (e.g., python3, protobuf, toml, etc.)
{% block install %}{% endblock %}

# switch back to original app-image user
USER {{app_user}}

See also https://stackoverflow.com/a/63643361/6325371

In this case, you won't need any other modifications. In particular, you won't need /gramine/app_files changes.

Or did I miss something?

We can't switch back to normal user immediately after block install. As, we will need root permissions further down in the code for something like https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L40

Thanks.

[dimakuv]

Reviewed 1 of 1 files at r2, 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 (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @veenasai2)

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

Previously, veenasai2 wrote…

We can't switch back to normal user immediately after block install. As, we will need root permissions further down in the code for something like https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L40

Thanks.

Hm, why do we need root permissions at that point? What is the error message?

[veenasai2]

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

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Hm, why do we need root permissions at that point? What is the error message?

First point where it fails https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L29

with an error message
/bin/sh: 1: cannot create /trusted_argv: Permission denied

This error comes because non-root user image does not have permissions to do something like mkdir -p /some_dir, however mkdir -p {working_dir}/some_dir is permitted.

If I bypass first error, then second error at https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L34

with an error message
ln: failed to create symbolic link './bash': Permission denied

[here second error does not halt the execution flow]

and the third error at https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L40

with an error message
chmod: changing permissions of '/apploader.sh': Operation not permitted

Thanks.

[dimakuv]

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

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

Previously, veenasai2 wrote…

First point where it fails https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L29

with an error message
/bin/sh: 1: cannot create /trusted_argv: Permission denied

This error comes because non-root user image does not have permissions to do something like mkdir -p /some_dir, however mkdir -p {working_dir}/some_dir is permitted.

If I bypass first error, then second error at https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L34

with an error message
ln: failed to create symbolic link './bash': Permission denied

[here second error does not halt the execution flow]

and the third error at https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.build.template#L40

with an error message
chmod: changing permissions of '/apploader.sh': Operation not permitted

Thanks.

Ah, so this is because we're putting all files in the root directory and doing everything in this root directory. Which is not accessible by a non-root user. Makes sense.

So the way you circumvent this is to create a /gramine/app_files/ directory and re-assign it to the non-root user.

But still, why can't you do these operations (that require the root user) in one code block and then switch back to normal user? something like this:

# temporarily switch to the root user to install packages
USER root

# Install distro-specific packages to run Gramine (e.g., python3, protobuf, toml, etc.)
{% block install %}{% endblock %}

# Create a directory that will store apploader.sh and entrypoint files.
RUN mkdir -p /gramine/app_files

# Make the app image user owner of /gramine/app_files directory
RUN chown {{app_user}} /gramine/app_files/

# switch back to original app-image user
USER {{app_user}}

This should be enough, no?

My main point with these proposed changes is that I don't like dragging the root user until the very end. I think we should switch to the normal user at the first possible moment.

[veenasai2]

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

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Ah, so this is because we're putting all files in the root directory and doing everything in this root directory. Which is not accessible by a non-root user. Makes sense.

So the way you circumvent this is to create a /gramine/app_files/ directory and re-assign it to the non-root user.

But still, why can't you do these operations (that require the root user) in one code block and then switch back to normal user? something like this:

# temporarily switch to the root user to install packages
USER root

# Install distro-specific packages to run Gramine (e.g., python3, protobuf, toml, etc.)
{% block install %}{% endblock %}

# Create a directory that will store apploader.sh and entrypoint files.
RUN mkdir -p /gramine/app_files

# Make the app image user owner of /gramine/app_files directory
RUN chown {{app_user}} /gramine/app_files/

# switch back to original app-image user
USER {{app_user}}

This should be enough, no?

My main point with these proposed changes is that I don't like dragging the root user until the very end. I think we should switch to the normal user at the first possible moment.

Yeah, your approach is better. In that case we need to make some additional changes as well , like

COPY --chown={{app_user}} *.py /
COPY --chown={{app_user}} apploader.sh /gramine/app_files/
COPY --chown={{app_user}} entrypoint.manifest /gramine/app_files/

otherwise again we will see

chmod: changing permissions of '/gramine/app_files/apploader.sh': Operation not permitted .

For now, I am not pushing the suggested changes as we have one more hidden dependency in finalize_manifest.py (for non-root user) at https://github.com/gramineproject/gsc/blob/master/finalize_manifest.py#L107 and https://github.com/gramineproject/gsc/blob/master/finalize_manifest.py#L85 that translates to an error

subprocess.CalledProcessError: Command 'ldconfig -v' returned non-zero exit status 1.

I will resolve this error and push the changes.

Thanks.

[aneessahib]

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

---

templates/entrypoint.common.manifest.template line 40 at r3 (raw file):

  "file:/etc/shadow-",
]

So you run into issues during the signing step, since the non root user does not have access to these files. By adding them allowed_files in manifest would result in skipping being added as trusted, resulting in a smooth signing stage, but I'm not sure if this is the right usage of allowed_files. May be other potential solutions

1. If user does not have access while signing, then ignore just go to the next file. This change would have to go into the sign tool, but this could be considered hacky.
2. Add something like sgx.skip_file , that GSC would ignore during the trusted file generation. This could be later used to optimize the size of large GSC images too.

@dimakuv what do you think

[dimakuv]

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

---

finalize_manifest.py line 38 at r3 (raw file):

                                r'|\.dockerenv'
                                r'|\.dockerinit'
                                r'|etc/mtab'

The new files to ignore should go into this list, not into sgx.allowed_files in template/entrypoint.common.manifest.template

---

finalize_manifest.py line 85 at r3 (raw file):

def generate_library_paths():
    encoding = sys.stdout.encoding if sys.stdout.encoding is not None else 'UTF-8'
    ld_paths = subprocess.check_output('ldconfig -v -N -X', stderr=subprocess.PIPE, shell=True)

FYI: Explaining this small change for future readers.

The previous version ldconfig -v had a side-effect of updating the caches and symlinks of the most recently used shared libraries. But we simply want to read the list of system paths where the libraries can be found; we don't want any side-effects.

So we had this bug previously, and never noticed it, because we only allowed root users -- and they have all privileges. So ldconfig -v silently updated the system-wide information.

Now that we allow non-root users, we hit this problem because ldconfig -v requires root privileges to update the caches. So we added -N -X to prevent the side-effects: -N forbids to rebuild the cache, -X forbids to update links.

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

Previously, veenasai2 wrote…

Yeah, your approach is better. In that case we need to make some additional changes as well , like

COPY --chown={{app_user}} *.py /
COPY --chown={{app_user}} apploader.sh /gramine/app_files/
COPY --chown={{app_user}} entrypoint.manifest /gramine/app_files/

otherwise again we will see

chmod: changing permissions of '/gramine/app_files/apploader.sh': Operation not permitted .

For now, I am not pushing the suggested changes as we have one more hidden dependency in finalize_manifest.py (for non-root user) at https://github.com/gramineproject/gsc/blob/master/finalize_manifest.py#L107 and https://github.com/gramineproject/gsc/blob/master/finalize_manifest.py#L85 that translates to an error

subprocess.CalledProcessError: Command 'ldconfig -v' returned non-zero exit status 1.

I will resolve this error and push the changes.

Thanks.

This is resolved, right? Please mark it as "Done".

---

templates/entrypoint.common.manifest.template line 40 at r3 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

So you run into issues during the signing step, since the non root user does not have access to these files. By adding them allowed_files in manifest would result in skipping being added as trusted, resulting in a smooth signing stage, but I'm not sure if this is the right usage of allowed_files. May be other potential solutions

1. If user does not have access while signing, then ignore just go to the next file. This change would have to go into the sign tool, but this could be considered hacky.
2. Add something like sgx.skip_file , that GSC would ignore during the trusted file generation. This could be later used to optimize the size of large GSC images too.

@dimakuv what do you think

Yes, we already have the function in our trusted file generation script (finalize_manifest.py), so we should use option 2.

Please delete the files from here and add them to the list pointed to by my other comment.

Btw, since that list uses regex, you can replace these:

  "file:/etc/gshadow",
  "file:/etc/gshadow-",

with a single line like this:

r'|etc/gshadow.*'

(It means "all files under etc/ that start with gshadow prefix")

[aneessahib]

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

---

templates/entrypoint.common.manifest.template line 40 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, we already have the function in our trusted file generation script (finalize_manifest.py), so we should use option 2.

Please delete the files from here and add them to the list pointed to by my other comment.

Btw, since that list uses regex, you can replace these:

  "file:/etc/gshadow",
  "file:/etc/gshadow-",

with a single line like this:

r'|etc/gshadow.*'

(It means "all files under etc/ that start with gshadow prefix")

Yes this had crossed our minds too, but this would remove access for root users as well wouldnt it? Do we want that?

[veenasai2]

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

---

templates/Dockerfile.common.build.template line 14 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is resolved, right? Please mark it as "Done".

Yes, it resolved. Done.

[dimakuv]

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

---

templates/entrypoint.common.manifest.template line 40 at r3 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

Yes this had crossed our minds too, but this would remove access for root users as well wouldnt it? Do we want that?

Looking at the list of files, they all pertain to passwords/user names. No real-world application that users would want to run in GSC/Gramine needs these files. Moreover, Gramine simply doesn't have any functionality to work with these files.

So yes, we can definitely remove access to these files, even for root users. I see no problems with it.

[veenasai2]

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

---

finalize_manifest.py line 38 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The new files to ignore should go into this list, not into sgx.allowed_files in template/entrypoint.common.manifest.template

done, thanks

---

templates/entrypoint.common.manifest.template line 40 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Looking at the list of files, they all pertain to passwords/user names. No real-world application that users would want to run in GSC/Gramine needs these files. Moreover, Gramine simply doesn't have any functionality to work with these files.

So yes, we can definitely remove access to these files, even for root users. I see no problems with it.

The suggested changes are done. Thanks.

[dimakuv]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @veenasai2)

---

finalize_manifest.py line 41 at r4 (raw file):

                                r'|etc/gshadow.*'
                                r'|etc/mtab'
                                r'|etc/.pwd.lock'

You need to escape dots, so it should look like this:

r'|etc/\.pwd\.lock'

---

finalize_manifest.py line 43 at r4 (raw file):

                                r'|etc/.pwd.lock'
                                r'|etc/rc(\d|.)\.d/.*'
                                r'|etc/security/opasswd'

Actually, I think you should just skip the whole directory:

r'|etc/security/.*'

[veenasai2]

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

---

finalize_manifest.py line 41 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You need to escape dots, so it should look like this:

r'|etc/\.pwd\.lock'

done, thanks.

---

finalize_manifest.py line 43 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, I think you should just skip the whole directory:

r'|etc/security/.*'

done, thanks.

[dimakuv]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all 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 @aneessahib, @dimakuv, and @veenasai2)

a discussion (no related file):
The PR looks good to me. Could you verify on 2-3 examples (one with non-root user, the others classic with root user) this final version?

---

[veenasai2]

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

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The PR looks good to me. Could you verify on 2-3 examples (one with non-root user, the others classic with root user) this final version?

@dimakuv, I have verified this final version on (1) python image from Docker hub (root user), (2) bash image (gsc/test) (root user), (3) hello-world image (gsc/test) with explicitly putting non-root user, (4) bitnami-pytorch image from docker hub (non-root user). Thanks.

---

[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 @aneessahib and @dimakuv)

a discussion (no related file):

Previously, veenasai2 wrote…

@dimakuv, I have verified this final version on (1) python image from Docker hub (root user), (2) bash image (gsc/test) (root user), (3) hello-world image (gsc/test) with explicitly putting non-root user, (4) bitnami-pytorch image from docker hub (non-root user). Thanks.

Nice, thanks for verification!

---

[mkow]

Reviewed 2 of 6 files at r1, 1 of 1 files at r2, 2 of 4 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 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 @aneessahib, @dimakuv, and @veenasai2)

---

gsc.py line 130 at r5 (raw file):

    user = config['User']
    if not user:
        user = "root"

Suggestion:

'root'

---

templates/Dockerfile.common.build.template line 11 at r5 (raw file):

FROM {{app_image}}

# App image "user" is stored, and execution will continue as root to install packages.

Why is this in doublequotes?

Code quote:

App image "user"

---

templates/Dockerfile.common.build.template line 13 at r5 (raw file):

# App image "user" is stored, and execution will continue as root to install packages.
# In the signed graminized image, the "user" will be restored.
ENV app_user={{app_user}}

Suggestion:

ENV APP_USER=

---

templates/Dockerfile.common.build.template line 13 at r5 (raw file):

# App image "user" is stored, and execution will continue as root to install packages.
# In the signed graminized image, the "user" will be restored.
ENV app_user={{app_user}}

Are you even using this env variable anywhere?

Code quote:

app_user

[veenasai2]

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

---

gsc.py line 130 at r5 (raw file):

    user = config['User']
    if not user:
        user = "root"

done, thanks.

---

templates/Dockerfile.common.build.template line 11 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is this in doublequotes?

I wanted to emphasize the user here. But anyways, I removed the entire comment as the env variable is no longer used. Thanks.

---

templates/Dockerfile.common.build.template line 13 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Are you even using this env variable anywhere?

No , we are not using this anymore. In the initial proposal I was using it. Thanks.

---

templates/Dockerfile.common.build.template line 13 at r5 (raw file):

# App image "user" is stored, and execution will continue as root to install packages.
# In the signed graminized image, the "user" will be restored.
ENV app_user={{app_user}}

Removed the app_user env variable. Thanks.

[dimakuv]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

[mkow]

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

---

templates/Dockerfile.common.build.template line 11 at r5 (raw file):

Previously, veenasai2 wrote…

I wanted to emphasize the user here. But anyways, I removed the entire comment as the env variable is no longer used. Thanks.

Doublequotes are used to cite something, for emphasis you should use * or _.

[veenasai2]

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

---

templates/Dockerfile.common.build.template line 11 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Doublequotes are used to cite something, for emphasis you should use * or _.

ok, thanks.

[dimakuv]

Reviewed 2 of 2 files at r7, 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) (waiting on @aneessahib)

---

-- commits line 2 at r1:

Previously, veenasai2 wrote…

Sure, thanks.

Done

---

gsc.py line 395 at r7 (raw file):

        # Copy sigstruct file from Docker container into temporary directory on the host
        docker_socket.containers.run(args.image,
            '\'cp /gramine/app_files/entrypoint.sig /tmp/host/ 2>/dev/null || :\'',

FYI: Noticed this small bug when manually testing (with ./gsc info-image gsc-python).

[dimakuv]

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

a discussion (no related file):
@veenasai2 This PR is ready for merge. I tested it on a couple classic images (Python, Bash). Could you also test it on your Bitnami Pytorch image, to be absolutely sure? If your test is successful, then we merge.

---

[mkow]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @aneessahib and @veenasai2)

[veenasai2]

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

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@veenasai2 This PR is ready for merge. I tested it on a couple classic images (Python, Bash). Could you also test it on your Bitnami Pytorch image, to be absolutely sure? If your test is successful, then we merge.

@dimakuv , Just now I tested the PR on Bitnami Pytorch image that has a non-root user. The PR is working fine, except the entrypoint.sig part.

---

---

-- commits line 2 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done

Thanks

---

gsc.py line 395 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

FYI: Noticed this small bug when manually testing (with ./gsc info-image gsc-python).

The path for the entrypoint.sig needs to be updated. I will upload the changes soon. Thanks.

[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: ITL) (waiting on @aneessahib and @dimakuv)

---

gsc.py line 395 at r7 (raw file):

Previously, veenasai2 wrote…

The path for the entrypoint.sig needs to be updated. I will upload the changes soon. Thanks.

@veenasai2 Wait, but I fixed it. What do you mean? Are you sure you used my latest version? I updated it just today. so that it works.

[veenasai2]

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

---

gsc.py line 395 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@veenasai2 Wait, but I fixed it. What do you mean? Are you sure you used my latest version? I updated it just today. so that it works.

@dimakuv I tried with your commit now, getting the below error for the bitnami pytorch image.

with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpvwz8smtw/entrypoint.sig'

However, with ./gsc info-image gsc-python , no errors. Thanks

[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: ITL) (waiting on @aneessahib and @dimakuv)

---

gsc.py line 395 at r7 (raw file):

Previously, veenasai2 wrote…

@dimakuv I tried with your commit now, getting the below error for the bitnami pytorch image.

with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpvwz8smtw/entrypoint.sig'

However, with ./gsc info-image gsc-python , no errors. Thanks

What is producing the error? Which command? Can you copy the whole output? I don't get it, you first say that there is an error, but then you say that there is no error. So which one is it?

[dimakuv]

Reviewable status: all 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: ITL) (waiting on @aneessahib, @dimakuv, and @veenasai2)

[veenasai2]

Reviewable status: all 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: ITL) (waiting on @aneessahib and @dimakuv)

---

gsc.py line 395 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is producing the error? Which command? Can you copy the whole output? I don't get it, you first say that there is an error, but then you say that there is no error. So which one is it?

./gsc info-image gsc-base-bitnami-image
Traceback (most recent call last):
  File "./gsc", line 12, in <module>
    sys.exit(main(sys.argv))
  File "./gsc.py", line 481, in main
    return args.command(args)
  File "./gsc.py", line 399, in gsc_info_image
    with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp90t3j4q1/entrypoint.sig'

 ./gsc info-image gsc-python
mr_enclave = "XXXXXXXXXXXXXXXXXX"
mr_signer = "XXXXXXXXXXXXXXXXXXX"
isv_prod_id = X
isv_svn = X
date = "2022-06-15"
flags = "XXXXXXXXXXXXX"
xfrms = "XXXXXXXXXXXXXX"
misc_select = "XXXXXXXXXXXXX"
debug = false

[veenasai2]

Reviewable status: all 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: ITL) (waiting on @aneessahib and @dimakuv)

---

gsc.py line 395 at r7 (raw file):

Previously, veenasai2 wrote…

./gsc info-image gsc-base-bitnami-image
Traceback (most recent call last):
  File "./gsc", line 12, in <module>
    sys.exit(main(sys.argv))
  File "./gsc.py", line 481, in main
    return args.command(args)
  File "./gsc.py", line 399, in gsc_info_image
    with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp90t3j4q1/entrypoint.sig'

 ./gsc info-image gsc-python
mr_enclave = "XXXXXXXXXXXXXXXXXX"
mr_signer = "XXXXXXXXXXXXXXXXXXX"
isv_prod_id = X
isv_svn = X
date = "2022-06-15"
flags = "XXXXXXXXXXXXX"
xfrms = "XXXXXXXXXXXXXX"
misc_select = "XXXXXXXXXXXXX"
debug = false

From ubuntu:18.04

RUN apt-get update

CMD ["bash"]

USER 1001

if we convert the bash image ( which is in gsc/test folder) to non-root user image (let say bash-non-root-image )
, we can see this issue happening.

./gsc info-image gsc-bash-non-root-image
Traceback (most recent call last):
  File "./gsc", line 12, in <module>
    sys.exit(main(sys.argv))
  File "./gsc.py", line 481, in main
    return args.command(args)
  File "./gsc.py", line 399, in gsc_info_image
    with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp3mm3aocl/entrypoint.sig'

Basically, with non-root user image, I am seeing this issue.

[veenasai2]

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

---

gsc.py line 395 at r7 (raw file):

Previously, veenasai2 wrote…

From ubuntu:18.04

RUN apt-get update

CMD ["bash"]

USER 1001

if we convert the bash image ( which is in gsc/test folder) to non-root user image (let say bash-non-root-image )
, we can see this issue happening.

./gsc info-image gsc-bash-non-root-image
Traceback (most recent call last):
  File "./gsc", line 12, in <module>
    sys.exit(main(sys.argv))
  File "./gsc.py", line 481, in main
    return args.command(args)
  File "./gsc.py", line 399, in gsc_info_image
    with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp3mm3aocl/entrypoint.sig'

Basically, with non-root user image, I am seeing this issue.

@dimakuv This issue is fixed now. I tested the new version with python image (root user) and bitnami image (non-root user). Thanks.

[dimakuv]

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

a discussion (no related file):

Previously, veenasai2 wrote…

@dimakuv , Just now I tested the PR on Bitnami Pytorch image that has a non-root user. The PR is working fine, except the entrypoint.sig part.

OK, ./gsc info-image on a non-root-user Docker image works now.

---

---

gsc.py line 395 at r7 (raw file):

Previously, veenasai2 wrote…

@dimakuv This issue is fixed now. I tested the new version with python image (root user) and bitnami image (non-root user). Thanks.

For the context: the issue stems from three facts:

• Python's TemporaryDirectory() creates a temporary directory with permissions 644 (owner can read and write, everyone else can only read).
• The root-user Docker image operates under root and can write to any directory anyway.
• The non-root-user Docker image operates under a UID that can be different from the UID of the temporary directory (in this case, Docker user is 1001 and host user is 1000). So the Docker user cannot write to the directory owned by the host user. Error happens.

The fix proposed by @veenasai2 is to allow everyone to write into the directory. Since this is a temporary directory created solely for this purpose, and is deleted afterwards, I see no problems with this approach.

---

gsc.py line 394 at r8 (raw file):

    with tempfile.TemporaryDirectory() as tmpdirname:
        # Grant temporary directory all the permissions for non-root user image access
        os.chmod(tmpdirname,0o777)

This should be 666 (we don't allow executing files). I will fix it during the final rebase. And we'll need to test it then again.

[dimakuv]

Reviewed 1 of 1 files at r9, 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) (waiting on @aneessahib)

---

gsc.py line 394 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This should be 666 (we don't allow executing files). I will fix it during the final rebase. And we'll need to test it then again.

Done.

[veenasai2]

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) (waiting on @aneessahib)

---

gsc.py line 394 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

@dimakuv, thanks for pushing these changes. I tested the latest version, here are the results:

./gsc info-image gsc-python gives following error with 666 (though works fine 777):

  File "./gsc.py", line 404, in gsc_info_image
    with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
PermissionError: [Errno 13] Permission denied: '/tmp/tmpy1tza3yr/entrypoint.sig'

same error , I got for bitnami image too.

I will do more analysis on why this is happening and get back soon. Thanks

[veenasai2]

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) (waiting on @aneessahib)

gsc.py line 394 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@dimakuv, thanks for pushing these changes. I tested the latest version, here are the results:

./gsc info-image gsc-python gives following error with 666 (though works fine 777):

  File "./gsc.py", line 404, in gsc_info_image
    with open(os.path.join(tmpdirname, "entrypoint.sig"), 'rb') as sig:
PermissionError: [Errno 13] Permission denied: '/tmp/tmpy1tza3yr/entrypoint.sig'

same error , I got for bitnami image too.

I will do more analysis on why this is happening and get back soon. Thanks

Based on this answer (3rd point) , looks like we need execute bit too set in order to enter into a directory and read a file.

[dimakuv]

Reviewed 1 of 1 files at r10, 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) (waiting on @aneessahib)

---

gsc.py line 394 at r8 (raw file):

Previously, veenasai2 wrote…

Based on this answer (3rd point) , looks like we need execute bit too set in order to enter into a directory and read a file.

Oops, you are right. Done, reverted back to your 777.

I tested on the classic Python image, it works fine. @veenasai2 @please test on your non-root-user image, and we should finally merge this PR.

[veenasai2]

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) (waiting on @aneessahib)

---

gsc.py line 394 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Oops, you are right. Done, reverted back to your 777.

I tested on the classic Python image, it works fine. @veenasai2 @please test on your non-root-user image, and we should finally merge this PR.

@dimakuv I tested again the final version on non-root image (bitnami pytorch).

./gsc build, sign-image, info-image, docker run gsc-image , all are working fine. Thanks.

[mkow]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aneessahib)

[dimakuv]

Dismissed @aneessahib from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved