fix(providers): allow git clone/fetch via default GitHub provider

[russellb]

Summary

The default GitHub provider blocked gh repo clone / git clone https://… inside sandboxes. This makes clone/fetch work while keeping push blocked by default.

Related Issue

Closes #1769

Changes

• providers/github.yaml: replace the access: read-only preset on the github.com:443 git-transport endpoint with explicit rules — GET/HEAD/OPTIONS ** plus POST /**/git-upload-pack. The read-only preset only expands to GET/HEAD/OPTIONS, but git smart HTTP needs POST to */git-upload-pack for clone/fetch.
• Push (git-receive-pack) stays denied; enabling it still requires an explicit policy proposal. The two api.github.com REST/GraphQL endpoints remain read-only.
• Tests: add a provider-profile regression test (openshell-providers) and a rego enforcement test (openshell-supervisor-network) covering ref discovery, git-upload-pack (allowed), and git-receive-pack (denied); update the two existing tests that asserted "all github endpoints read-only".

Why allowing this POST is still read-only

In git's smart HTTP protocol, POST is an RPC transport, not a write. A clone/fetch is GET */info/refs (ref discovery) followed by POST */git-upload-pack, whose body is only the client's want/have negotiation; the server replies with a packfile and nothing on the server is modified (data flows server → client). The service names are from the server's perspective: git-upload-pack = the server uploads a pack to the client (a read/download), while git-receive-pack = the server receives a pack from the client (the actual write/push). The new rule is scoped to */git-upload-pack only, so push and arbitrary POSTs to github.com remain denied.

Testing

• openshell-providers, openshell-supervisor-network, and openshell-server targeted tests pass (new + updated regression tests green)
• Unit tests added/updated
• E2E tests added/updated (verified at the L7 policy-enforcement layer via a rego test rather than full sandbox e2e)

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[johntmyers]

Thanks @russellb lgtm at first glance. Having Gator do its thing anyway.

[johntmyers]

gator-agent

PR Review Status

Validation: This PR directly implements confirmed bug #1769 with a concentrated Provider V2/L7 policy change. The proposed git-upload-pack exception preserves the intended read-only Git boundary; I found no current authorization bypass or privilege escalation.

Head SHA: 756183525eeab149594af0919db0eab2a4a5e1cf

Thanks @johntmyers. I checked the implementation beyond your first-glance LGTM, including canonical path handling, OPA matching, provider-policy composition, and the existing Fern guidance. Three author changes remain:

• Strengthen the built-in-profile regression in crates/openshell-providers/src/profiles.rs so it asserts the exact allowed rule set (GET/HEAD/OPTIONS ** and POST /**/git-upload-pack) and fails if a broad or additional POST rule could enable push. The current substring checks would still pass alongside POST **. Ideally, evaluate the actual composed built-in endpoint through OPA instead of relying only on the hand-copied fixture in crates/openshell-supervisor-network/src/opa.rs.
• Add the real clone/fetch E2E requested in the trusted triage on bug: gh repo clone fails with default github provider — POST git-upload-pack denied #1769. The current unit test does not exercise provider attachment, effective-policy composition, TLS interception, redirects, or real Git behavior.
• Update the existing Fern provider documentation (for example, docs/sandboxes/providers-v2.mdx) to say that the built-in GitHub profile permits HTTPS clone/fetch through git-upload-pack while API mutations and git-receive-pack remain denied. No navigation change appears necessary.

Docs: Missing for this direct user-visible default-provider behavior change.

Next state: gator:in-review. Please push an updated commit addressing these items. On the new head, gator will re-review and make the required test:e2e/copy-PR authorization decision.

[russellb]

Pushed afb0205 addressing the three items.

1. Exact rule-set regression. github_git_transport_allows_clone_but_not_push (providers) now pins the whole allowed set — GET/HEAD/OPTIONS ** + POST /**/git-upload-pack — via a sorted assert_eq!, plus asserts no access preset and no deny rules. A POST ** (or any extra POST) now fails instead of slipping past the substring check. Applied the same exact-set assertion to the composed-layer test provider_policy_layers_include_known_provider_profiles (server), since the "broad/extra POST could enable push" concern applies as much to the effective policy. I did not add the in-crate "evaluate through OPA" variant: openshell-providers doesn't depend on the OPA engine crate and the profile→effective-policy composition lives in the server, so there's no natural home for it below the e2e — which now covers that path for real (see #2).

2. Real clone/fetch e2e. Added test_github_provider_allows_https_git_clone (e2e/python/test_sandbox_providers.py): enables providers_v2, attaches the built-in github provider, and git clones a public repo over HTTPS, asserting success + .git/HEAD. Exercises provider attachment, effective-policy composition, TLS interception, and real git (the git-remote-https helper is covered via /usr/bin/git ancestor matching). Verified green against an ephemeral Podman-backed gateway. Scoped to clone-success; push-denied stays covered by the rego test.

3. Docs. Updated docs/sandboxes/providers-v2.mdx — both github.com YAML examples now show the explicit clone/fetch rules instead of the stale read-only preset, with prose stating clone/fetch via git-upload-pack is permitted while API mutations and git-receive-pack remain denied.

[johntmyers]

gator-agent

PR Review Status

Validation: This PR remains project-valid because it directly fixes confirmed Provider V2/L7 bug #1769 while keeping Git push denied by default.

Head SHA: afb0205f6978abca804e2951b18fba065ee9d21a

Thanks @russellb. I verified the three updates you described: the profile and composed-policy tests now pin the exact allow set, the new E2E attaches the built-in GitHub provider and performs a real HTTPS clone, and the existing Fern provider page now explains clone/fetch versus push. Those prior requests are resolved.

Review findings:

• One blocking E2E isolation issue remains in the inline review: the helper deletes a gateway-global setting without preserving its prior state and without guarding concurrent workers.

Docs: Updated in docs/sandboxes/providers-v2.mdx; no navigation change is needed because the page is already under the existing sandboxes navigation folder.

Next state: gator:in-review. Please restore the setting's exact prior value/presence and serialize this gateway-global mutation, then push a new head for re-review.

[johntmyers]

gator-agent

Warning: This unconditionally deletes the gateway-global providers_v2_enabled setting. When E2E targets an existing or shared gateway where the setting was explicitly true or false, this leaves the gateway altered and can affect concurrent or subsequent tests. Read the prior state with GetGatewayConfig, restore its exact value or absence in finally, and ensure this gateway-global mutation cannot race parallel E2E workers (for example, with a serialized suite fixture or a dedicated gateway).

[russellb]

Fixed in dc397b8. _providers_v2_enabled is now a fixture that:

• Reads prior state via GetGatewayConfig and restores the exact value or absence in finally. GetGatewayConfig returns known keys even when unset (empty SettingValue), so it treats the setting as present only when the value oneof is actually set; on a gateway where it was unset, teardown deletes it rather than leaving it altered.
• Serializes the gateway-global mutation across xdist workers with an exclusive fcntl.flock on the run's shared base temp dir (getbasetemp().parent), held for the whole test so the read-modify-restore can't interleave.

Verified green end-to-end (clone + clean teardown) against an ephemeral Podman-backed gateway.

[johntmyers]

gator-agent

PR Review Status

Validation: This PR remains project-valid because it fixes confirmed Provider V2/L7 bug #1769 with a narrow smart-HTTP upload-pack rule while keeping push and arbitrary POSTs denied.

Head SHA: dc397b892cd00b890aec3a4f9aa7cb3dc257a7bf

Thanks @russellb. I checked the isolation update you described. Reading GetGatewayConfig and restoring the exact prior value or absence resolves the state-restoration half of the prior finding. The remaining concurrency issue is in the inline review: the advisory lock is acquired only by this fixture, while other xdist workers use the same gateway without acquiring it and can observe the temporary global setting.

Review findings:

• One blocking E2E isolation finding remains in the batched inline review.

Security: The exact provider/composed-policy assertions and OPA coverage preserve default deny: git-upload-pack is allowed, while git-receive-pack and unrelated POSTs remain denied. Credential handling is unchanged.

Docs: Updated in docs/sandboxes/providers-v2.mdx; no navigation change is needed because the existing folder navigation already includes this page.

Next state: gator:in-review. Please use a suite-wide shared/exclusive gateway-config guard or run this test against an isolated gateway/serial phase, then push a new head for re-review.

[johntmyers]

gator-agent

Warning (CWE-362): This lock only coordinates tests that acquire the same lock, but no other xdist tests do. Every worker connects to the same gateway, so another worker can create a provider-attached sandbox while providers_v2_enabled is transiently true and receive a different effective policy; a concurrent external change can also be overwritten during restoration. Please use a suite-wide autouse shared/exclusive gateway-config guard, or run this test against an isolated gateway/serial phase. The exact prior-value restoration here is otherwise correct.

[russellb]

Fixed in b1d100b. Replaced the per-fixture lock with a suite-wide readers-writer guard so the exclusivity actually covers other workers:

• New autouse _gateway_config_guard in e2e/python/conftest.py: every test takes a shared flock on a gateway-config lock file in the run's shared base temp dir; a test marked exclusive_gateway_config takes an exclusive lock instead.
• The clone test is marked exclusive_gateway_config, so while it enables and restores providers_v2_enabled no other xdist worker is mid-test — none can create a sandbox and observe the transient global. providers_v2_enabled depends on the guard so the exclusive lock is held before the mutation.
• Exact prior-value/absence restoration via GetGatewayConfig is retained.

Deadlock isn't possible by construction (one lock per test, always released at test end, no nesting). Verified green against an ephemeral Podman gateway: the exclusive clone test passes, and the shared-lock path runs without guard error on the other provider tests.