security: env var allowlist + testable OCI config builder#2
Open
jw409 wants to merge 24 commits intofreedomofpress:mainfrom
Open
security: env var allowlist + testable OCI config builder#2jw409 wants to merge 24 commits intofreedomofpress:mainfrom
jw409 wants to merge 24 commits intofreedomofpress:mainfrom
Conversation
This is not required anymore, as we will be able to reference multiple container images after the first two are built.
Rename the `--only-local` flag to `--local` when running tests locally.
Replaces the denylist of non-forwarded env vars with an allowlist of just
LANG, LC_ALL, LC_CTYPE, LANGUAGE, and TZ. Any other var in the outer
container's environment is dropped entirely -- neither its name nor its
value appears in the OCI config written to gVisor.
Why drop rather than mask with '<ENV>=':
- Allowlist semantics are stricter: we forward exactly what conversion
needs and nothing else. Masking implies we still enumerate what to
hide; dropping means the name is never in the config file.
- Sensitive names (cloud provider tokens, SSH/GPG agent paths, k8s
config locations) can themselves be signals. Not writing them at all
is one fewer leak surface.
- gVisor behavior is identical either way: it reads the config's env
array; keys not present simply don't exist inside the sandbox.
Factors the OCI config build into build_oci_config(command, env) so the
full config can be tested, not just the allowlist set in isolation. The
test asserts on the final config['process']['env'] array -- the actual
data reaching gVisor -- and covers baseline presence, allowlist forward,
sensitive names AND values absent, sandbox-control vars dropped, parent
cannot shadow baseline, and hostile values (newlines/nulls/shell inj)
dropped when key is not allowlisted.
Tests live at tests/helpers/test_entrypoint.py, mirroring the source
layout under src/helpers/.
Closed
2 tasks
Member
|
Hi, this repository is under heavy works, and so I've force-pushed a few times the Now, to the actual changes. Please keep your changes minimal the much you can: if you're doing an allow-list, do only that and avoid making other changes to the code so that it's easier to review, and also doesn't introduce changes in other parts of the code. This is important as this is a security-sensitive project. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/helpers/entrypoint.pywith an allowlist of justLANG,LC_ALL,LC_CTYPE,LANGUAGE, andTZ. Any other var in the outer container's environment is dropped entirely — neither name nor value appears in the OCI config that gVisor consumes.build_oci_config(command, env)so the finalconfig["process"]["env"]array can be tested directly, not just the allowlist set in isolation.tests/helpers/test_entrypoint.py(20 tests, all passing locally) mirroring thesrc/helpers/source layout.Context
This PR ports the env-var hardening from freedomofpress/dangerzone#1454 to this repo, per @apyrgio's guidance that container-image code now lives here. Review comments on 1454 that shaped this rewrite:
<ENV>=behavior" — went with drop instead; see rationale below.build_oci_config()is now a pure function the test drives directly.tests/container_helpers/to mirror sources" — done astests/helpers/to match this repo's layout.Why drop rather than mask with
<ENV>=AWS_SECRET_ACCESS_KEY,SSH_AUTH_SOCK,KUBECONFIG,ANTHROPIC_API_KEY— leaking the key name tells an attacker inside the sandbox what the outer context is running. Drop removes that channel.runscreadsconfig["process"]["env"]; keys not present simply don't exist inside the sandbox. Verified by inspection of the runtime spec.Test plan
pytest tests/helpers/test_entrypoint.py -v --local— 20/20 passRUNSC_DEBUG/RUNSC_FLAGS) dropped, parent cannot shadow baselinePATH/PYTHONPATH/TERM, hostile values (newlines, nulls, shell injection, embedded=) dropped when key is not allowlisted.build_oci_config()is pure — no module-level side effects — so the test loadsentrypoint.pyviaimportlibwithout invokingmain().Supersedes freedomofpress/dangerzone#1454 (moved to draft there, per repo split).