Workflow sandbox #164

cretz · 2022-10-25T15:29:23Z

What was changed

This is better described in the README, but high-level notes:

Created entirely new temporalio.worker.workflow_sandbox package
Changed default runner to this new sandbox
Uses exec() for global state isolation
Uses custom importer
Lots of Python libraries do lots of side-effecting, non-deterministic things on import so we have lots of knobs for affecting what is/isn't restricted
Users can skip restricting code using with workflow.unsafe.sandbox_unrestricted():
Users can skip sandbox per workflow via @workflow.defn(sandboxed=False)
Users can skip sandbox per worker via runner_class=UnsandboxedWorkflowRunner (what the default is before this PR)
Has caveats such as improperly affecting global state which we need to fix
TODO: Need to try subinterpreter via Rust
TODO: Test for performance

It is quite possible people's code is gonna break. The sandbox is a bit fickle with how it checks restrictions and such. But if you have a simple workflow trying to call random or open a file, this will stop you.

Closes #80

README.md

temporalio/worker/workflow_sandbox/importer.py

Sushisource · 2022-10-25T21:02:09Z

README.md

+To remove restrictions around a particular block of code, use `with temporalio.workflow.unsafe.sandbox_unrestricted():`.
+The workflow will still be running in the sandbox, but no restrictions for invalid library calls will be applied.


That's nice

README.md

Sushisource · 2022-10-25T21:13:38Z

temporalio/worker/workflow_sandbox/restrictions.py

+    return ret
+
+
+# TODO(cretz): Should I make this more declarative in an external file?


I think this is fine honestly, especially if you could change it slightly to allow passing just a string list for all the all_uses cases, which would reduce the verbosity a lot.

Yeah, I may make it a bit easier to write. I am not happy with my SandboxMatcher and I have learned a lot since, but I marked this entire API as unstable, so I can clean later.

Sushisource · 2022-10-25T21:15:55Z

temporalio/worker/workflow_sandbox/restrictions.py

+    def __getattribute__(self, __name: str) -> Any:
+        state = _RestrictionState.from_proxy(self)
+        _trace("__getattribute__ %s on %s", __name, state.name)
+        state.assert_child_not_restricted(__name)
+        ret = object.__getattribute__(self, "__getattr__")(__name)


This might be the highest density of underscores I've ever seen in a code block lol

It's definitely confusing. It's how you prevent accidental recursion when accessing self after you have overridden __getattribute__ (ref https://docs.python.org/3/reference/datamodel.html#object.__getattribute__). In this case I'm accessing __getattr__ the higher level of __getattribute__, and __name is named that way to prevent you from using it as a kwarg (signature taken from typeshed basically IIRC)

Sushisource · 2022-10-25T21:48:19Z

temporalio/workflow.py

+        sandboxed: Whether the workflow should run in a sandbox. Default is
+            true.


Would make sense to refer to more detailed docs about what the sandbox is here.

I wanted to, but I am afraid of linking to the README considering the goal would be to have it in real docs (but the README is so much more comprehensive). I have this same problem in workflow_sandbox/__init__.py where I just say:

See the Python SDK documentation for how to use, customize, and work around sandbox issues.

tests/worker/workflow_sandbox/test_runner.py

Sushisource

This makes sense to me. Definitely... tedious, though. Would be curious to see if we could end up with less code via the multiple interpreters route, but I suppose all the blacklisting stuff can only ever get so short.

cretz · 2022-10-25T22:34:26Z

This makes sense to me. Definitely... tedious, though. Would be curious to see if we could end up with less code via the multiple interpreters route

I think I can simplify in a couple of ways:

Use https://github.com/GrahamDumpleton/wrapt instead of this thing I copied from https://github.com/pallets/werkzeug/blob/a3005e6acda7246fe0a684c71921bf4882b4ba1c/src/werkzeug/local.py#L382
Use https://docs.python.org/3/c-api/init.html#sub-interpreter-support via Rust (e.g. https://pyo3.rs/main/doc/pyo3_ffi/fn.py_newinterpreter) and see if I can still pass through libs

I think it also will be safer. But I didn't have enough time to continue my pursuits here. My other concern is perf and mem utilization. So I need to test this sandbox first.

I suppose all the blacklisting stuff can only ever get so short.

Yeah, I don't mind maintaining a blacklist of stdlib. I think it's also gonna have use being accessed programmatically when I make a static analyzer.

README.md

bergundy

I got about half way through the review.
I'll pick it back up tomorrow.
So far LGTM, didn't have any critical blocking comments.

bergundy · 2022-10-25T23:36:21Z

temporalio/worker/replayer.py

@@ -49,7 +50,8 @@ def __init__(
        *,
        workflows: Sequence[Type],
        workflow_task_executor: Optional[concurrent.futures.ThreadPoolExecutor] = None,
-        workflow_runner: WorkflowRunner = UnsandboxedWorkflowRunner(),
+        workflow_runner: WorkflowRunner = SandboxedWorkflowRunner(),
+        unsandboxed_workflow_runner: WorkflowRunner = UnsandboxedWorkflowRunner(),


Is this a stable option? Should it be marked experimental?

I think it's a stable option. It is what is run when you do @workflow.defn(sandboxed=False).

bergundy · 2022-10-25T23:37:04Z

temporalio/worker/worker.py

+            unsandboxed_workflow_runner: Runner for workflows that opt-out of
+                sandboxing.


Same here, is this a stable API?

I believe so, yes

temporalio/worker/workflow_sandbox/importer.py

bergundy · 2022-10-26T00:37:59Z

temporalio/worker/workflow_sandbox/in_sandbox.py

+logger = logging.getLogger(__name__)
+
+# Set to true to log lots of sandbox details
+LOG_TRACE = False


nit: merge this with the other LOG_TRACE var?

Which one? I think these should remain file-specific

temporalio/worker/workflow_sandbox/runner.py

Quinn-With-Two-Ns · 2022-10-27T20:47:12Z

temporalio/worker/workflow_sandbox/runner.py

+        for k, v in extra_globals.items():
+            self.globals_and_locals[k] = v
+        try:
+            exec(code, self.globals_and_locals, self.globals_and_locals)


I know we didn't test for performance, but do you think compiling and caching the workflow code could be beneficial?

Probably not much. This runs only a few lines of code. Two of those are static yes (another is dynamic), but the actual user's workflow code is already cached by Python in the loader and import just re-executes it. The compile() step here would only compile those few lines. But yes, upon benchmarking, this may be worth it. I can make that code static too. I may come back to this after profiling.

cretz added 11 commits September 27, 2022 14:34

Initial work on sandbox

d935689

Work on module restrictions

fc4634a

Rework object proxying approach

85e4cd4

Work on stdlib restrictions

49332e8

Update sandbox matcher and add decorator option

d1cbed8

More work on stdlib restrictions

1c036bf

Begin sandbox rewrite

70b0672

Completed importer

bc83389

Working sandbox

11c7ba2

Support for import-time vs runtime restrictions

27b8fbe

Work on sandbox documentation

c6f2fb5

cretz requested a review from a team October 25, 2022 15:29

cretz mentioned this pull request Oct 25, 2022

[Feature Request] Tests for Python 3.11 support #165

Closed

cretz added 2 commits October 25, 2022 11:23

Python 3.7 fix

d0dfe0e

Merge branch 'main' into workflow-sandbox

25d5ff6

cretz marked this pull request as ready for review October 25, 2022 17:30

cretz commented Oct 25, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

temporalio/worker/workflow_sandbox/importer.py Outdated Show resolved Hide resolved

Downgrade CI to 3.10.7

1a0da34

This was referenced Oct 25, 2022

[Feature Request] Support FailureConverter concept #142

Closed

[Feature Request] Apply codecs to Failure.encodedAttributes #131

Closed

Sushisource reviewed Oct 25, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Sushisource reviewed Oct 25, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Sushisource reviewed Oct 25, 2022

View reviewed changes

tests/worker/workflow_sandbox/test_runner.py Outdated Show resolved Hide resolved

Sushisource approved these changes Oct 25, 2022

View reviewed changes

bergundy reviewed Oct 25, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

bergundy approved these changes Oct 26, 2022

View reviewed changes

cretz added 2 commits October 26, 2022 14:24

Move to thread-local sys.modules and builtin part

47a9932

CI fix and move test

2313157

cretz commented Oct 26, 2022

View reviewed changes

temporalio/worker/workflow_sandbox/runner.py Outdated Show resolved Hide resolved

cretz commented Oct 26, 2022

View reviewed changes

temporalio/worker/workflow_sandbox/runner.py Outdated Show resolved Hide resolved

cretz mentioned this pull request Oct 27, 2022

Don't require timer presence when fired #169

Merged

Recursion fix and minor updates

e7755f7

cretz force-pushed the workflow-sandbox branch from 403798d to e7755f7 Compare October 27, 2022 16:30

cretz requested a review from bergundy October 27, 2022 16:35

Make global attr alteration atomic and only undone when last is finish

7e66172

Quinn-With-Two-Ns approved these changes Oct 27, 2022

View reviewed changes

Quinn-With-Two-Ns reviewed Oct 27, 2022

View reviewed changes

cretz merged commit 0126e6e into temporalio:main Oct 28, 2022

cretz deleted the workflow-sandbox branch October 28, 2022 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow sandbox #164

Workflow sandbox #164

cretz commented Oct 25, 2022

Sushisource Oct 25, 2022

Sushisource Oct 25, 2022

cretz Oct 25, 2022

Sushisource Oct 25, 2022

cretz Oct 25, 2022 •

edited

Loading

Sushisource Oct 25, 2022

cretz Oct 25, 2022

Sushisource left a comment

cretz commented Oct 25, 2022 •

edited

Loading

bergundy left a comment

bergundy Oct 25, 2022

cretz Oct 26, 2022

bergundy Oct 25, 2022

cretz Oct 26, 2022

bergundy Oct 26, 2022

cretz Oct 26, 2022

Quinn-With-Two-Ns Oct 27, 2022

cretz Oct 27, 2022

		To remove restrictions around a particular block of code, use `with temporalio.workflow.unsafe.sandbox_unrestricted():`.
		The workflow will still be running in the sandbox, but no restrictions for invalid library calls will be applied.

		return ret


		# TODO(cretz): Should I make this more declarative in an external file?

		sandboxed: Whether the workflow should run in a sandbox. Default is
		true.

		unsandboxed_workflow_runner: Runner for workflows that opt-out of
		sandboxing.

Workflow sandbox #164

Workflow sandbox #164

Conversation

cretz commented Oct 25, 2022

What was changed

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

cretz Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource left a comment

Choose a reason for hiding this comment

cretz commented Oct 25, 2022 • edited Loading

bergundy 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

cretz Oct 25, 2022 •

edited

Loading

cretz commented Oct 25, 2022 •

edited

Loading