Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 71 additions & 5 deletions crates/openshell-providers/src/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
55 changes: 49 additions & 6 deletions crates/openshell-server/src/grpc/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
}

Expand Down
55 changes: 55 additions & 0 deletions crates/openshell-supervisor-network/src/opa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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#"
Expand Down
14 changes: 12 additions & 2 deletions docs/sandboxes/providers-v2.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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
Expand Down
135 changes: 135 additions & 0 deletions e2e/python/test_sandbox_providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

from __future__ import annotations

import fcntl
import time
from contextlib import contextmanager
from typing import TYPE_CHECKING
Expand Down Expand Up @@ -99,6 +100,74 @@ def _delete_provider(stub: object, name: str) -> None:
raise


@pytest.fixture
def providers_v2_enabled(
sandbox_client: SandboxClient,
tmp_path_factory: pytest.TempPathFactory,
) -> 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, so this mutates shared state. To keep it safe
on an existing or shared gateway and under parallel workers:

- The mutation is serialized across xdist workers with an exclusive file lock
on the run's shared base temp dir (``getbasetemp().parent``), held for the
whole test so the read-modify-restore cannot interleave.
- The exact prior value (or absence) is captured via ``GetGatewayConfig`` and
restored in ``finally``, leaving the gateway as it was found.

``global`` is a Python keyword, so it is passed through a dict expansion.
"""
stub = sandbox_client._stub
key = "providers_v2_enabled"
lock_path = tmp_path_factory.getbasetemp().parent / "providers-v2-setting.lock"
with lock_path.open("w") as lock_file:
fcntl.flock(lock_file, fcntl.LOCK_EX)
Comment thread
johntmyers marked this conversation as resolved.
Outdated

# GetGatewayConfig returns known keys even when unset, with an empty
# SettingValue (no populated oneof). Only treat the setting as explicitly
# present when its value oneof is actually set; otherwise restore = delete.
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
# ===========================================================================
Expand Down Expand Up @@ -484,3 +553,69 @@ 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.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}"
)
Loading
Loading