feat: Configure codejail and run safety check at startup #10

timmc-edx · 2025-02-06T11:43:33Z

Initialize codejail at startup, if CODE_JAIL is set
Run safety checks at startup, locking out the API if the checks fail

If codejail isn't properly configured, it defaults to running code unsafely. To prevent this from affecting the service, we run a smoke test at startup to check if there's anything just drastically wrong.

If this check does not pass, two things happen:

The healthcheck endpoint will never return a 200 OK
The code-exec endpoint will refuse with a 500 error

Supporting changes:

Define an explicit AppConfig for the api subpackage so that we can hook into the ready() mechanism
Wrap safe_exec to prevent codejail eagerly setting UNSAFE=True at module load time. (Not clear why this doesn't affect edx-platform; maybe something to do with app vs. middleware load order.) Filed Codejail safe_exec makes "unsafe=true" decision at startup codejail#225 for possibly fixing this.
safe_exec wrapper also performs a deepcopy to allow callers to reason about the globals dict more easily.

Other changes:

Clean up healthcheck docstring (mostly just trim it down)
Lint cleanup

Part of edx/edx-arch-experiments#927

Manual testing performed with changes to the Dockerfile and to devstack (PRs pending), and mostly entailed calling the healthcheck endpoint.

When passing, the startup logs look like this:

edx.devstack.codejail  | 2025-02-05 23:26:03,745 INFO 365 [codejail_service.startup_check] [user None] [ip None] startup_check.py:73 - Startup test 'Basic code execution' passed
edx.devstack.codejail  | 2025-02-05 23:26:03,819 INFO 365 [codejail_service.startup_check] [user None] [ip None] startup_check.py:73 - Startup test 'Block sandbox escape by disk access' passed
edx.devstack.codejail  | 2025-02-05 23:26:03,892 INFO 365 [codejail_service.startup_check] [user None] [ip None] startup_check.py:73 - Startup test 'Block sandbox escape by child process' passed

When codejail is misconfigured:

edx.devstack.codejail  | 2025-02-06 11:47:41,056 WARNING 419 [codejail] [user None] [ip None] safe_exec.py:305 - Using codejail/safe_exec.py:not_safe_exec for None
edx.devstack.codejail  | 2025-02-06 11:47:41,058 INFO 419 [codejail_service.startup_check] [user None] [ip None] startup_check.py:73 - Startup test 'Basic code execution' passed
edx.devstack.codejail  | 2025-02-06 11:47:41,058 WARNING 419 [codejail] [user None] [ip None] safe_exec.py:305 - Using codejail/safe_exec.py:not_safe_exec for None
edx.devstack.codejail  | 2025-02-06 11:47:41,059 ERROR 419 [codejail_service.startup_check] [user None] [ip None] startup_check.py:76 - Startup test 'Block sandbox escape by disk access' failed with: "Expected error, but code ran successfully. Globals: {'ret': ['var', 'home', 'lib64', 'tmp', 'boot', 'media', 'root', 'etc', 'srv', 'proc', 'usr', 'run', 'bin', 'dev', 'opt', 'lib', 'sbin', 'mnt', 'sys', 'edx', '.dockerenv', 'app', 'venv', 'sandbox', 'lib.usr-is-merged']}"
edx.devstack.codejail  | 2025-02-06 11:47:41,059 WARNING 419 [codejail] [user None] [ip None] safe_exec.py:305 - Using codejail/safe_exec.py:not_safe_exec for None
edx.devstack.codejail  | 2025-02-06 11:47:41,061 ERROR 419 [codejail_service.startup_check] [user None] [ip None] startup_check.py:76 - Startup test 'Block sandbox escape by child process' failed with: "Expected error, but code ran successfully. Globals: {'ret': '42\\n'}"

Merge checklist:
Check off if complete or not applicable:

Version bumped
Changelog record added
Documentation updated (not only docstrings)
Unit tests added/updated
Manual testing instructions provided
Noted any: Concerns, dependencies, migration issues, deadlines, tickets

- Initialize codejail at startup, if `CODE_JAIL` is set - Run safety checks at startup, locking out the API if the checks fail If codejail isn't properly configured, it defaults to running code unsafely. To prevent this from affecting the service, we run a smoke test at startup to check if there's anything just *drastically* wrong. If this check does not pass, two things happen: - The healthcheck endpoint will never return a 200 OK - The code-exec endpoint will refuse with a 500 error Supporting changes: - Define an explicit AppConfig for the api subpackage so that we can hook into the `ready()` mechanism - Wrap `safe_exec` to prevent codejail eagerly setting `UNSAFE=True` at module load time. (Not clear why this doesn't affect edx-platform; maybe something to do with app vs. middleware load order.) Filed openedx/codejail#225 for possibly fixing this. - `safe_exec` wrapper also performs a deepcopy to allow callers to reason about the globals dict more easily. Other changes: - Clean up healthcheck docstring (mostly just trim it down) - Lint cleanup Part of edx/edx-arch-experiments#927

robrap

Thanks.

robrap · 2025-02-06T19:31:56Z

codejail_service/apps/api/apps.py

+        else:  # pragma: no cover
+            # Codejail needs this at startup
+            apply_django_settings(settings.CODE_JAIL)


What would happen if this were broken? Would some unit test fail, or is there a reason why we don't want coverage for this?

If it broke, the service would fail to start. There isn't currently any unit test coverage, because we can't really configure codejail outside of a container.

...but it looks like I can set CODE_JAIL = {} in the settings/test.py, and that still allows tests to pass. I thought I had previously seen a failure with an empty settings block, but I can't reproduce that now. I'll add it in.

robrap · 2025-02-06T19:39:50Z

codejail_service/apps/core/tests/test_views.py

 from django.test import TestCase
 from django.urls import reverse


 class HealthTests(TestCase):
    """Tests of the health endpoint."""

-    def test_healthcheck(self):
+    @patch('codejail_service.startup_check.STARTUP_SAFETY_CHECK_OK', None)
+    def test_unhealthy(self):
        """Test that the endpoint reports when all services are healthy."""


Docstring for test_unhealthy and test_healthy need to be swapped.

Thanks, will fix. Also going to add a couple more cases.

robrap · 2025-02-06T20:48:43Z

codejail_service/tests/test_startup_check.py

+        (responses(math=Exception("Divide by zero")), False),
+    )
+    @ddt.unpack
+    @patch('codejail_service.startup_check.STARTUP_SAFETY_CHECK_OK', None)


I thought this ends up as a parameter on the test? I'm confused about how this works.

That surprised me too. But if you set the new parameter here, you don't get an additional argument to the decorated function.

If patch() is used as a decorator and new is omitted, the created mock is passed in as an extra argument to the decorated function.

Which is the new parameter? The None? Maybe I'm just used to patched functions, and that is what is throwing me off.

Yeah, this sets codejail_service.startup_check.STARTUP_SAFETY_CHECK_OK to None for the duration of the test and then sets it back to the original value afterwards.

robrap · 2025-02-06T20:54:48Z

codejail_service/tests/test_startup_check.py

+        mock_log_info.assert_has_calls([
+            call("Startup test 'Basic code execution' passed"),
+        ])
+        assert (
+            "Startup test 'Block sandbox escape by disk access' failed with: "
+            "\"Expected error, but code ran successfully. Globals: {'ret': ['"
+        ) in mock_log_error.call_args_list[0][0][0]
+        assert (
+            "Startup test 'Block sandbox escape by child process' failed with: "
+            r'''"Expected error, but code ran successfully. Globals: {'ret': '42\\n'}"'''
+        ) == mock_log_error.call_args_list[1][0][0]


Do you intentionally not want to check the count of the various log messages, to ensure there is nothing else?

Not specifically, no. I mostly wanted to ensure that all of the useful information was present. I could add an additional check if you think it makes sense to do so.

The reason for it is that it might ensure you add proper coverage if a new validation test and log message were ever added.

I suppose so. I mostly care whether there's at least one failure message and at least one success message, but no harm in adding some count assertions.

robrap · 2025-02-06T21:03:30Z

codejail_service/startup_check.py

+    STARTUP_SAFETY_CHECK_OK = not any_failed
+
+
+def _test_basic_function():


Is there any non-mocked test for this function, and is there any way to? Same for the other calls.

Yes -- test_logging and test_unsafe_tests_default both expect these to actually run.

I see. Although maybe this should have been obvious, maybe in test_logging you could explain that because we don't actually have any protections enabled via AppArmor in the unit test, we can assume that all safety tests should fail.

Can you confirm that we don't have any tests that pass the safety tests by mocking at the safe_exec level? If not, would this be useful?
UPDATE: I guess that is what the first test in test_failure_modes is, maybe, which isn't a failure? I'm not following that test well. UPDATE 2: Maybe call it test_success_and_failure_modes?

Sure, I can add that note to the test.

Having trouble understanding the second para.

- Set CODE_JAIL to empty dict during unit tests, which allows us to always call `apply_django_settings` instead of having an uncovered branch. - Fix docstrings for healthcheck unit tests Also: - Cover additional cases in healthcheck tests

timmc-edx force-pushed the timmc/safe-startup branch from edd7062 to 6479eca Compare February 6, 2025 11:46

timmc-edx mentioned this pull request Feb 6, 2025

Secure codejail code-exec edx/edx-arch-experiments#927

Open

9 tasks

timmc-edx force-pushed the timmc/safe-startup branch from 92f7b43 to a0de742 Compare February 6, 2025 17:40

timmc-edx force-pushed the timmc/safe-startup branch from 2123d53 to e1ae885 Compare February 6, 2025 17:55

timmc-edx marked this pull request as ready for review February 6, 2025 18:12

robrap reviewed Feb 6, 2025

View reviewed changes

timmc-edx added 2 commits February 6, 2025 21:28

fixup! Fixes from review

a3bd908

- Set CODE_JAIL to empty dict during unit tests, which allows us to always call `apply_django_settings` instead of having an uncovered branch. - Fix docstrings for healthcheck unit tests Also: - Cover additional cases in healthcheck tests

fixup! Assert number of log messages

87800de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Configure codejail and run safety check at startup #10

feat: Configure codejail and run safety check at startup #10

timmc-edx commented Feb 6, 2025 •

edited

Loading

robrap left a comment

robrap Feb 6, 2025

timmc-edx Feb 6, 2025

robrap Feb 6, 2025

timmc-edx Feb 6, 2025

robrap Feb 6, 2025

timmc-edx Feb 6, 2025

robrap Feb 6, 2025

timmc-edx Feb 6, 2025

robrap Feb 6, 2025

timmc-edx Feb 6, 2025

robrap Feb 6, 2025

timmc-edx Feb 6, 2025

robrap Feb 6, 2025

timmc-edx Feb 6, 2025

robrap Feb 6, 2025 •

edited

Loading

timmc-edx Feb 6, 2025

		STARTUP_SAFETY_CHECK_OK = not any_failed


		def _test_basic_function():

feat: Configure codejail and run safety check at startup #10

Are you sure you want to change the base?

feat: Configure codejail and run safety check at startup #10

Conversation

timmc-edx commented Feb 6, 2025 • edited Loading

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmc-edx commented Feb 6, 2025 •

edited

Loading

robrap Feb 6, 2025 •

edited

Loading