feat: AppArmor confinement for codejail-service #109

timmc-edx · 2025-02-13T00:24:37Z

Allow codejail-service to actually run code (and run it securely) by giving it the needed confinement. Previously it would run but refuse to execute code since it would detect the insecure environment; now, the startup safety checks pass and the code-exec endpoint works as expected.

Add an AppArmor profile with fairly strict rules. It needs to be thoroughly vetted and to have exceptions added before it can be used in production, but it's be fine for devstack. Some parts are based on the existing edxapp apparmor config without careful review.
Apply the profile to the codejail service in docker-compose.
Add Django configs for codejail service.
Add documentation for installing the profile so that it is available for use on the dev's machine.

Also:

Add configuration and documentation for edxapp to actually call the codejail service, disabled by default. (Will later want to make this default to true, once the service is working properly.)
Update image name in docker-compose to follow rename in Several codejail-service cleanups public-dockerfiles#102

Currently edxapp gets an error back from codejail-service, and then isn't able to read that error; separate work in the app repo will be needed to fix those. (The first issue relates to python_path, and the other to not returning globals_dict when there's an emsg.) But the integration is working otherwise.

I've completed each of the following or determined they are not applicable:

Made a plan to communicate any major developer interface changes (or N/A)

Allow codejail-service to actually run code (and run it securely) by giving it the needed confinement. Previously it would run but refuse to execute code since it would detect the insecure environment; now, the startup safety checks pass and the code-exec endpoint works as expected. - Add an AppArmor profile with fairly strict rules. It needs to be thoroughly vetted and to have exceptions added before it can be used in production, but it's be fine for devstack. Some parts are based on the existing edxapp apparmor config without careful review. - Apply the profile to the codejail service in docker-compose. - Add Django configs for codejail service. - Add documentation for installing the profile so that it is available for use on the dev's machine. Also: - Add configuration and documentation for edxapp to actually call the codejail service, disabled by default. (Will later want to make this default to true, once the service is working properly.) - Update image name in docker-compose to follow rename in edx/public-dockerfiles#102 Currently edxapp gets an error back from codejail-service, and then isn't able to read that error; separate work in the app repo will be needed to fix those. (The first issue relates to python_path, and the other to not returning globals_dict when there's an emsg.) But the integration is working otherwise.

timmc-edx · 2025-02-13T00:27:41Z

py_configuration_files/lms.py

+# Disabled by default since codejail service needs to be configured
+# and started separately. See docs/codejail.rst for details.
+ENABLE_CODEJAIL_REST_SERVICE = False
+CODE_JAIL_REST_SERVICE_HOST = "http://edx.devstack.codejail:8080"


Author's note: I had thought the gunicorn port wouldn't matter and I could just set it to a conventional value, but I forgot it would be used by inter-service communication in devstack. This makes for an odd mismatch with the rest of the ports seen in devstack. Maybe it's fine, though.

You could leave a code comment:

The port does not match the pattern used for other devstack services because...

codejail.profile

py_configuration_files/cms.py

We can use either `#include` or `include` and it might be less confusing to use the one that doesn't look like a comment. I've added comments to some directives that I now understand better.

Just about every sample policy on the web includes attach_disconnected but the manpage describes it as a debugging tool that is not safe for general use.

robrap · 2025-02-14T15:00:48Z

docs/codejail.rst

+Both LMS and CMS can run Python code submitted by instructors and learners in order to implement custom Python-graded problems. By default this involves running the code on the same host as edxapp itself. Ordinarily this would be quite dangerous, but we use a sandboxing library called `codejail <https://github.com/openedx/codejail>`__ in order to confine the code execution in terms of disk and network access as well as memory, CPU, and other resource limits. Part of these restrictions are implemented via AppArmor, a utility available in some Linux distributions (including Debian and Ubuntu).
+
+While AppArmor provides good protection, a sandbox escape could still be possible due to misconfiguration or bugs in AppArmor. For defense in depth, we're setting up a dedicated `codejail service <https://github.com/openedx/codejail-service>`__ that will perform code execution for edxapp and which will allow further isolation.
+
+The default edxapp codejail defaults to unsafe, direct execution of Python code, and this remains true in devstack. We don't even have a way to run on-host codejail securely in devstack. In constrast, the codejail service refuses to run if codejail has not been configured properly, and we've included a way to run it in devstack.


[opinion] I'd leave most of this for the openedx/codejail-service docs. It's going to change even more if the old behaviour is DEPRed (including removal). Here, I'd write more from the perspective that we have and want a codejail-service, and here is what is needed to work with it.

This is non-blocking. It is an opinion to minimize future doc updates as time passes.

Note: I could imagine an ADR that captures this in the codejail-service, if that doesn't already exist, and you could point to it if you feel it is worth it, without surrounding it with much redundant text.

robrap · 2025-02-14T15:02:17Z

docs/codejail.rst

+
+In order to run the codejail devstack component:
+
+1. Install AppArmor: ``sudo apt install apparmor``


Is this directly on my local machine (Mac, etc.)? Could you make that more explicit?

Once I'm clear, I can run through this as well.

Yes, it's meant to be run on the host machine. Although now I'm realizing that I'm not actually sure where you're supposed to run it on a Mac! It's a Linux command, and won't work for you.

How does this docker stuff all work on Mac, anyhow? Maybe there's a Linux VM that all the docker stuff runs inside? If so, this would need to be run in that Linux VM.

Maybe this will help us? abiosoft/colima#1019

robrap · 2025-02-14T15:07:13Z

docs/codejail.rst

+
+Any time you update the profile, you'll need to re-run the command to apply the profile.
+
+The profile file contains the directive ``profile codejail_service``. That defines the name of the profile when it is installed into the kernel. In order to change that name, you must first remove the profile **under the old name**, then install a new profile under the new name. To remove a profile, use the ``--remove`` action instead of the ``-replace`` action: : ``sudo apparmor_parser --remove -W codejail.profile``


Is there supposed to be a relationship between the name codejail.profile and the earlier profile codejail_service? Can you better explain the connection?

Nope, filename is unrelated -- could be named anything. profile codejail_service is the important part. It tells apparmor to install a profile of this name in the OS.

I started the review with this doc, and I asked this before I had my bearings. I didn't realize that codejail.profile was a file under review as part of this PR. Maybe you could note earlier on where to find the file codejail.profile, and we can adjust the doc when and if it moves?

robrap · 2025-02-14T15:09:18Z

docs/codejail.rst

+
+Changes to the AppArmor profile must be coordinated with changes to the Dockerfile, as they need to agree on filesystem paths.
+
+Any time you update the profile, you'll need to re-run the command to apply the profile.


I was wondering how to do this. Is the next paragraph the answer? If so, maybe either start the next paragraph with "To re-run the command to apply the profile...", or just make this one paragraph that goes together. Or, any other way to clarify what how you to this. Thanks.

This was supposed to mean that the command to install the profile in the first place and the command to update it are one and the same. (It's the one with --replace.) It's a kind of confusing situation. I'll try to clarify the docs.

(Actually, I think you can use --add instead of --replace, but then every time afterwards you need to use --replace, which... I'd rather just use one command for both!)

robrap · 2025-02-14T15:14:51Z

docs/codejail.rst

+
+The profile file contains the directive ``profile codejail_service``. That defines the name of the profile when it is installed into the kernel. In order to change that name, you must first remove the profile **under the old name**, then install a new profile under the new name. To remove a profile, use the ``--remove`` action instead of the ``-replace`` action: : ``sudo apparmor_parser --remove -W codejail.profile``
+
+The profile name must also agree with the relevant ``security_opt`` line in devstack's ``docker-compose.yml``.


I'm not sure exactly what this means.

Is this whole section about changing the name meant to be instructions for a permanent name change/improvement, or a useful development technique? If so, maybe you could explain why and when you'd want to change the name.

It sounds like there might be certain steps to take when changing a name. For example, removing the old profile name. Do you need to do anything to add the new name, or does remove do everything you want? Do you then need to update the security_opt line? If so, this would be better written: "Next, you must update the relevant security_opt line...`. If this is not what you meant at all, maybe this is enough to show you my confusion with this line? I'm not clear when and if you are asking me to change something, and which side to change.

It's basically supposed to be a warning that changing the name is not simple. I should probably replace this with something like "avoid changing the name, and here's why", and people can look up the process elsewhere if they need it. I'm hoping we can stick with this name and not change it.

robrap · 2025-02-14T15:30:00Z

docs/codejail.rst

+
+Both LMS and CMS can run Python code submitted by instructors and learners in order to implement custom Python-graded problems. By default this involves running the code on the same host as edxapp itself. Ordinarily this would be quite dangerous, but we use a sandboxing library called `codejail <https://github.com/openedx/codejail>`__ in order to confine the code execution in terms of disk and network access as well as memory, CPU, and other resource limits. Part of these restrictions are implemented via AppArmor, a utility available in some Linux distributions (including Debian and Ubuntu).
+
+While AppArmor provides good protection, a sandbox escape could still be possible due to misconfiguration or bugs in AppArmor. For defense in depth, we're setting up a dedicated `codejail service <https://github.com/openedx/codejail-service>`__ that will perform code execution for edxapp and which will allow further isolation.


It may make sense to either change this to point to the README: https://github.com/openedx/codejail-service/blob/main/README.rst, and/or refer to reading those docs, rather than just noting the service.

robrap · 2025-02-14T15:37:20Z

codejail.profile

+        # includes the sandbox's copy of Python as well as any
+        # dependencies that have been installed for inclusion in
+        # sandboxes.
+        /sandbox/venv/** rm,


When I first read this, I got confused with the rm command. Maybe add a comment of "r - read, m - memory (or whatever this actually is"?
Note: I think rw is less confusing below, and having a single comment here would be good enough for removing confusion. I don't think we need to comment all the lines below in this way.

robrap · 2025-02-14T15:38:33Z

codejail.profile

+    # apparmor will continue to make policy decisions in cases where a confined
+    # executable has a handle to a file's inode even after the file is removed
+    # from the filesystem.
+    profile child flags=(mediate_deleted) {


[proposal] Again, using the name child_profile will make this more clear throughout other code and docs for those that are less familiar.

robrap · 2025-02-14T15:42:39Z

py_configuration_files/codejail.py

+CODEJAIL_ENABLED = True
+
+CODE_JAIL = {
+    # These values are coordinated with the Dockerfile and AppArmor profile


Maybe something like this would help people find these references?

Suggested change

# These values are coordinated with the Dockerfile and AppArmor profile

# These values are coordinated with the Dockerfile (edx/public-dockerfiles) and AppArmor profile (codejail.profile)

robrap · 2025-02-14T15:45:11Z

py_configuration_files/lms.py

+# Disabled by default since codejail service needs to be configured
+# and started separately. See docs/codejail.rst for details.
+ENABLE_CODEJAIL_REST_SERVICE = False
+CODE_JAIL_REST_SERVICE_HOST = "http://edx.devstack.codejail:8080"


You could leave a code comment:

The port does not match the pattern used for other devstack services because...

- Document what netlink is and why it's needed - Allow TCP connections, but not establishing outbound ones - Split apart capability lines into two blocks for easier commenting