diff --git a/crates/openshell-providers/src/profiles.rs b/crates/openshell-providers/src/profiles.rs index 62809138cd..c770a1e9eb 100644 --- a/crates/openshell-providers/src/profiles.rs +++ b/crates/openshell-providers/src/profiles.rs @@ -1763,15 +1763,81 @@ mod tests { "github profile should include read-only GraphQL endpoint" ); assert!( - proto - .endpoints - .iter() - .all(|endpoint| endpoint.access == "read-only"), - "github profile endpoints should all be read-only" + proto.endpoints.iter().all(|endpoint| { + // The REST/GraphQL API endpoints stay read-only. The git + // transport endpoint (github.com) carries explicit rules + // instead so it can allow clone/fetch while blocking push. + if endpoint.host == "github.com" { + endpoint.access.is_empty() + } else { + endpoint.access == "read-only" + } + }), + "github API endpoints should be read-only; git transport uses explicit rules" ); assert_eq!(proto.binaries.len(), 4); } + #[test] + fn github_git_transport_allows_clone_but_not_push() { + let profile = builtin_profile("github"); + let proto = profile.to_proto(); + + let git_transport = proto + .endpoints + .iter() + .find(|endpoint| endpoint.host == "github.com" && endpoint.port == 443) + .expect("github.com git transport endpoint"); + + // The git transport carries explicit rules rather than an access preset + // (an empty preset would otherwise expand to GET/HEAD/OPTIONS). + assert!( + git_transport.access.is_empty(), + "git transport must use explicit rules, not an access preset" + ); + + // Assert the EXACT allowed rule set. Clone/fetch over git smart HTTP + // performs GET */info/refs (ref discovery) followed by POST + // */git-upload-pack. A substring check alone is not enough: a broader or + // additional POST rule (e.g. POST **) would also permit push via + // git-receive-pack while still passing a "some rule allows upload-pack" + // check. Pinning the whole set fails on any such regression. See #1769. + let mut allowed: Vec<(&str, &str)> = git_transport + .rules + .iter() + .map(|rule| { + let allow = rule + .allow + .as_ref() + .expect("git transport rules must be allow rules"); + (allow.method.as_str(), allow.path.as_str()) + }) + .collect(); + allowed.sort_unstable(); + + let mut expected = vec![ + ("GET", "**"), + ("HEAD", "**"), + ("OPTIONS", "**"), + ("POST", "/**/git-upload-pack"), + ]; + expected.sort_unstable(); + + assert_eq!( + allowed, expected, + "git transport allow rules must be exactly the read-only methods \ + plus POST */git-upload-pack (clone/fetch); a broader or extra POST \ + rule would enable push (git-receive-pack)" + ); + + // Blocking push must not depend on a deny rule, which could mask an + // over-broad allow and hide a regression. + assert!( + git_transport.deny_rules.is_empty(), + "git transport should block push via its narrow allow set, not deny rules" + ); + } + #[test] fn credential_env_vars_are_deduplicated_in_profile_order() { let profile = builtin_profile("claude-code"); diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 6fccf1eade..cb71ad5703 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -5116,12 +5116,55 @@ mod tests { "github provider policy should include read-only GraphQL endpoint" ); assert!( - layers[0] - .rule - .endpoints - .iter() - .all(|endpoint| endpoint.access == "read-only"), - "github provider policy should be read-only by default" + layers[0].rule.endpoints.iter().all(|endpoint| { + // API endpoints stay read-only; the github.com git transport + // carries explicit rules so clone/fetch works (see #1769). + if endpoint.host == "github.com" { + endpoint.access.is_empty() + } else { + endpoint.access == "read-only" + } + }), + "github API endpoints should be read-only; git transport uses explicit rules" + ); + // Pin the exact composed rule set for the git transport. Clone/fetch + // needs GET */info/refs then POST */git-upload-pack; a broader or extra + // POST rule (e.g. POST **) would also permit push (git-receive-pack), so + // matching the whole set fails on any such regression (see #1769). + let git_transport = layers[0] + .rule + .endpoints + .iter() + .find(|endpoint| endpoint.host == "github.com") + .expect("composed policy should include the github.com git transport"); + let mut allowed: Vec<(&str, &str)> = git_transport + .rules + .iter() + .map(|rule| { + let allow = rule + .allow + .as_ref() + .expect("git transport rules must be allow rules"); + (allow.method.as_str(), allow.path.as_str()) + }) + .collect(); + allowed.sort_unstable(); + let mut expected = vec![ + ("GET", "**"), + ("HEAD", "**"), + ("OPTIONS", "**"), + ("POST", "/**/git-upload-pack"), + ]; + expected.sort_unstable(); + assert_eq!( + allowed, expected, + "composed git transport allow rules must be exactly the read-only \ + methods plus POST */git-upload-pack; a broader POST rule would \ + enable push (git-receive-pack)" + ); + assert!( + git_transport.deny_rules.is_empty(), + "composed git transport should block push via its narrow allow set, not deny rules" ); } diff --git a/crates/openshell-supervisor-network/src/opa.rs b/crates/openshell-supervisor-network/src/opa.rs index 829c63caa4..3b9854d91e 100644 --- a/crates/openshell-supervisor-network/src/opa.rs +++ b/crates/openshell-supervisor-network/src/opa.rs @@ -3443,6 +3443,61 @@ network_policies: assert!(!eval_l7(&engine, &get_with_method)); } + // Mirrors the default GitHub provider's github.com git-transport endpoint + // (providers/github.yaml). Git smart HTTP clone/fetch performs a GET on + // */info/refs followed by a POST to */git-upload-pack; push uses + // */git-receive-pack, which must stay blocked. Regression test for #1769. + #[test] + fn l7_github_git_transport_allows_clone_blocks_push() { + let data = r#" +network_policies: + github: + name: github + endpoints: + - host: github.com + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } + binaries: + # l7_input() issues requests as /usr/bin/curl. + - { path: /usr/bin/curl } +"#; + let engine = OpaEngine::from_strings(TEST_POLICY, data).expect("engine from yaml"); + + // Reference discovery (GET) is allowed. + let refs = l7_input("github.com", 443, "GET", "/NVIDIA/OpenShell.git/info/refs"); + assert!(eval_l7(&engine, &refs), "GET info/refs should be allowed"); + + // Clone/fetch (POST git-upload-pack) is allowed. + let upload_pack = l7_input( + "github.com", + 443, + "POST", + "/NVIDIA/OpenShell.git/git-upload-pack", + ); + assert!( + eval_l7(&engine, &upload_pack), + "POST git-upload-pack should be allowed for clone/fetch" + ); + + // Push (POST git-receive-pack) is denied. + let receive_pack = l7_input( + "github.com", + 443, + "POST", + "/NVIDIA/OpenShell.git/git-receive-pack", + ); + assert!( + !eval_l7(&engine, &receive_pack), + "POST git-receive-pack (push) must be denied" + ); + } + #[test] fn l7_jsonrpc_request_params_do_not_affect_method_policy() { let data = r#" diff --git a/docs/sandboxes/providers-v2.mdx b/docs/sandboxes/providers-v2.mdx index c3c84b8383..df35b93534 100644 --- a/docs/sandboxes/providers-v2.mdx +++ b/docs/sandboxes/providers-v2.mdx @@ -605,11 +605,17 @@ endpoints: - host: github.com port: 443 protocol: rest - access: read-only enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } binaries: [/usr/bin/gh, /usr/local/bin/gh, /usr/bin/git, /usr/local/bin/git] ``` +The `github.com` git-transport endpoint uses explicit rules instead of the `read-only` preset so HTTPS clone and fetch work out of the box. Git smart HTTP performs a `GET` on `*/info/refs` followed by a `POST` to `*/git-upload-pack`; the read-only preset (`GET`/`HEAD`/`OPTIONS`) blocks that `POST`. The rules permit the read-only methods plus `POST */git-upload-pack` only, so clone and fetch succeed while push (`git-receive-pack`) and other API mutations stay denied. Enabling push requires an explicit policy proposal. + If a sandbox attaches a provider named `work-github`, the effective policy includes a generated provider rule: ```yaml wordWrap showLineNumbers={false} @@ -642,8 +648,12 @@ network_policies: - host: github.com port: 443 protocol: rest - access: read-only enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } binaries: - path: /usr/bin/gh - path: /usr/local/bin/gh diff --git a/e2e/python/conftest.py b/e2e/python/conftest.py index 07b7feb34b..45bd1ba4d1 100644 --- a/e2e/python/conftest.py +++ b/e2e/python/conftest.py @@ -3,6 +3,7 @@ from __future__ import annotations +import fcntl import os import time from typing import TYPE_CHECKING @@ -16,6 +17,38 @@ from collections.abc import Callable, Iterator +def pytest_configure(config: pytest.Config) -> None: + config.addinivalue_line( + "markers", + "exclusive_gateway_config: hold an exclusive lock on gateway-global " + "config so no other xdist worker observes a transient global setting", + ) + + +@pytest.fixture(autouse=True) +def _gateway_config_guard( + request: pytest.FixtureRequest, + tmp_path_factory: pytest.TempPathFactory, +) -> Iterator[None]: + """Readers-writer guard over gateway-global config mutations. + + The e2e gateway is shared across xdist workers, so a test that flips a + gateway-global setting can leak that transient value into another worker's + sandbox creation. Every test holds a shared lock by default; a test marked + ``exclusive_gateway_config`` holds an exclusive lock, so while it mutates and + restores the setting no other worker is mid-test and none can observe the + transient value. The lock file lives in the run's shared base temp dir + (``getbasetemp().parent``), which is common to all xdist workers. + """ + lock_path = tmp_path_factory.getbasetemp().parent / "gateway-config.lock" + exclusive = ( + request.node.get_closest_marker("exclusive_gateway_config") is not None + ) + with lock_path.open("w") as lock_file: + fcntl.flock(lock_file, fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH) + yield + + @pytest.fixture(scope="session") def cluster_name() -> str | None: return os.environ.get("OPENSHELL_GATEWAY") diff --git a/e2e/python/test_sandbox_providers.py b/e2e/python/test_sandbox_providers.py index 083a424921..21820f2e48 100644 --- a/e2e/python/test_sandbox_providers.py +++ b/e2e/python/test_sandbox_providers.py @@ -99,6 +99,68 @@ def _delete_provider(stub: object, name: str) -> None: raise +@pytest.fixture +def providers_v2_enabled( + sandbox_client: SandboxClient, + _gateway_config_guard: None, +) -> Iterator[None]: + """Enable the gateway-global ``providers_v2_enabled`` opt-in for one test. + + Composing a provider's network policy onto a sandbox is gated behind this + setting, which defaults off; the built-in github profile's git-transport + rules only reach the sandbox with it enabled. + + The setting is gateway-global. Exclusivity against other xdist workers is + provided by the ``exclusive_gateway_config`` marker plus the autouse + ``_gateway_config_guard`` guard (see conftest): no concurrent worker is + mid-test while this fixture mutates and restores the setting, so none can + observe the transient value. Depending on the guard here also orders the + exclusive lock acquisition before the mutation. + + ``GetGatewayConfig`` returns known keys even when unset, with an empty + ``SettingValue`` (no populated oneof), so the setting is treated as present + only when its value oneof is set; otherwise restore is a delete. ``global`` + is a Python keyword, so it is passed through a dict expansion. + """ + stub = sandbox_client._stub + key = "providers_v2_enabled" + config = stub.GetGatewayConfig(sandbox_pb2.GetGatewayConfigRequest()) + prior_value = sandbox_pb2.SettingValue() + had_prior = ( + key in config.settings + and config.settings[key].WhichOneof("value") is not None + ) + if had_prior: + prior_value.CopyFrom(config.settings[key]) + + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + setting_value=sandbox_pb2.SettingValue(bool_value=True), + **{"global": True}, + ) + ) + try: + yield + finally: + if had_prior: + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + setting_value=prior_value, + **{"global": True}, + ) + ) + else: + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + delete_setting=True, + **{"global": True}, + ) + ) + + # =========================================================================== # Tests: placeholder visibility # =========================================================================== @@ -484,3 +546,70 @@ def test_update_provider_rejects_type_change( assert "type cannot be changed" in exc_info.value.details() finally: _delete_provider(stub, name) + + +# =========================================================================== +# Tests: git transport network policy +# =========================================================================== + + +@pytest.mark.exclusive_gateway_config +@pytest.mark.usefixtures("providers_v2_enabled") +def test_github_provider_allows_https_git_clone( + sandbox: Callable[..., Sandbox], + sandbox_client: SandboxClient, +) -> None: + """Built-in github provider permits anonymous HTTPS clone/fetch (#1769). + + Git smart HTTP clone/fetch issues a POST to ``*/git-upload-pack``. The + read-only preset (GET/HEAD/OPTIONS) denied that POST, so ``git clone`` over + HTTPS failed. Attaching the github provider composes its network policy onto + the sandbox, exercising provider attachment, effective-policy composition, + TLS interception, and real git behavior end to end. git delegates HTTPS to a + ``git-remote-https`` helper whose ancestor is ``/usr/bin/git``, so the + profile's git binary covers it via ancestor matching. The + ``providers_v2_enabled`` fixture turns on the gateway-global gate that + composes the provider's network policy. + """ + with provider( + sandbox_client._stub, + name="e2e-test-github-clone", + provider_type="github", + # A required credential value is needed to create the provider, but an + # anonymous clone of a public repo never uses it: git only sends the + # token when a credential helper is configured. + credentials={"GITHUB_TOKEN": "e2e-placeholder-unused"}, + ) as provider_name: + # git opens /dev/null O_RDWR, so it must be read-write; the shared + # _default_policy only grants /dev/urandom. Everything else (binaries, + # CA bundle, clone target) is covered by the standard allowlist. + policy = _default_policy() + policy.filesystem.read_write.append("/dev/null") + spec = datamodel_pb2.SandboxSpec( + policy=policy, + providers=[provider_name], + ) + + with sandbox(spec=spec, delete_on_exit=True) as sb: + clone = sb.exec( + [ + "git", + "clone", + "--depth", + "1", + "https://github.com/octocat/Hello-World.git", + "/tmp/hello-world", + ], + timeout_seconds=120, + ) + assert clone.exit_code == 0, ( + "git clone over HTTPS should succeed with the github provider " + f"attached; stdout={clone.stdout!r} stderr={clone.stderr!r}" + ) + + # A completed clone materializes .git/HEAD, proving ref discovery + # (GET) and upload-pack (POST) both succeeded, not just a handshake. + head = sb.exec(["cat", "/tmp/hello-world/.git/HEAD"]) + assert head.exit_code == 0, ( + f"cloned repo is missing .git/HEAD; stderr={head.stderr!r}" + ) diff --git a/providers/github.yaml b/providers/github.yaml index 4ce5af2d31..4f23c2e3d8 100644 --- a/providers/github.yaml +++ b/providers/github.yaml @@ -29,10 +29,18 @@ endpoints: protocol: graphql access: read-only enforcement: enforce - # github.com is the git transport (clone / fetch by default). + # github.com is the git transport (clone / fetch by default). Git smart + # HTTP needs POST to */git-upload-pack for clone/fetch, which the + # read-only preset (GET/HEAD/OPTIONS) blocks. Spell the rules out so + # clone/fetch works while push (git-receive-pack) stays denied — enabling + # push requires an explicit policy proposal. - host: github.com port: 443 protocol: rest - access: read-only enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } binaries: [/usr/bin/gh, /usr/local/bin/gh, /usr/bin/git, /usr/local/bin/git]