fix(git-gate): make the gateway access-hook executable regardless of copy transport
test / unit (pull_request) Successful in 1m19s
test / integration (pull_request) Successful in 30s
test / coverage (pull_request) Successful in 1m35s
lint / lint (push) Successful in 2m35s
test / unit (push) Successful in 1m21s
test / integration (push) Successful in 29s
test / coverage (push) Successful in 1m31s
Update Quality Badges / update-badges (push) Successful in 1m21s
test / unit (pull_request) Successful in 1m19s
test / integration (pull_request) Successful in 30s
test / coverage (pull_request) Successful in 1m35s
lint / lint (push) Successful in 2m35s
test / unit (push) Successful in 1m21s
test / integration (push) Successful in 29s
test / coverage (push) Successful in 1m31s
Update Quality Badges / update-badges (push) Successful in 1m21s
Cloning/fetching from the git-gate on the Apple-container backend failed with
"empty reply from server" (curl exit 52). Root cause: the git-http handler
crashed on every upload-pack with
PermissionError: [Errno 13] Permission denied: '/etc/git-gate/access-hook'
The access-hook is exec'd directly, so it needs the x bit. prepare() stages it
0o700 and trusted the gateway copy to carry that mode. `docker cp` does; the
Apple `container cp` (AppleGatewayTransport) does not, landing the hook 0o644 →
EACCES. The unhandled exception killed the handler thread, closing the socket
with no HTTP response — which the client sees as the opaque empty reply.
- provision_git_gate now `chmod +x`es the access-hook on the gateway side after
the copy, so it's executable under every transport (docker/apple/firecracker).
- git-http handler wraps the access-hook subprocess.run: an OSError /
SubprocessError (un-execable, timed out) now fails closed with a 503 instead
of crashing the thread into an empty reply — a gate that can't run its hook
should deny, visibly.
- Updates the now-misleading "docker cp preserves source mode" comment in
git_gate.prepare().
Regression tests: provisioning applies +x to the access-hook; the handler
returns 503 (not an empty reply) when the hook can't be exec'd.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit was merged in pull request #403.
This commit is contained in:
@@ -94,6 +94,12 @@ def provision_git_gate(
|
|||||||
transport.exec(["mkdir", "-p", "/etc/git-gate"])
|
transport.exec(["mkdir", "-p", "/etc/git-gate"])
|
||||||
transport.cp_into(str(plan.hook_script), "/etc/git-gate/pre-receive")
|
transport.cp_into(str(plan.hook_script), "/etc/git-gate/pre-receive")
|
||||||
transport.cp_into(str(plan.access_hook_script), "/etc/git-gate/access-hook")
|
transport.cp_into(str(plan.access_hook_script), "/etc/git-gate/access-hook")
|
||||||
|
# The access-hook is exec'd directly (not via `sh`), so it needs the x bit.
|
||||||
|
# Set it here rather than trusting the copy to carry the staged 0o700:
|
||||||
|
# `docker cp` preserves source mode, but the Apple `container cp` does not,
|
||||||
|
# landing the hook 0o644 → EACCES when the git-http handler tries to exec it.
|
||||||
|
# chmod on the gateway side is backend-neutral and fixes every transport.
|
||||||
|
transport.exec(["chmod", "+x", "/etc/git-gate/access-hook"])
|
||||||
creds = _creds_dir(bottle_id)
|
creds = _creds_dir(bottle_id)
|
||||||
transport.exec(["mkdir", "-p", creds])
|
transport.exec(["mkdir", "-p", creds])
|
||||||
for u in plan.upstreams:
|
for u in plan.upstreams:
|
||||||
|
|||||||
@@ -112,8 +112,10 @@ class GitGate(ABC):
|
|||||||
access_hook = stage_dir / "git_gate_access_hook.sh"
|
access_hook = stage_dir / "git_gate_access_hook.sh"
|
||||||
access_hook.write_text(git_gate_render_access_hook())
|
access_hook.write_text(git_gate_render_access_hook())
|
||||||
# 0o700 (not 0o600): git daemon execs --access-hook directly,
|
# 0o700 (not 0o600): git daemon execs --access-hook directly,
|
||||||
# not via `sh`, so the script needs the x bit. docker cp
|
# not via `sh`, so the script needs the x bit. The gateway copy
|
||||||
# preserves source mode into the container.
|
# does not necessarily preserve this mode (`docker cp` does, the
|
||||||
|
# Apple `container cp` does not), so provision_git_gate re-applies
|
||||||
|
# +x on the gateway side — see backend/docker/gateway_provision.py.
|
||||||
access_hook.chmod(0o700)
|
access_hook.chmod(0o700)
|
||||||
upstreams_with_files: list[GitGateUpstream] = []
|
upstreams_with_files: list[GitGateUpstream] = []
|
||||||
for u in upstreams:
|
for u in upstreams:
|
||||||
|
|||||||
@@ -148,12 +148,24 @@ class GitHttpHandler(BaseHTTPRequestHandler):
|
|||||||
"GIT_GATE_ACCESS_HOOK", "/etc/git-gate/access-hook",
|
"GIT_GATE_ACCESS_HOOK", "/etc/git-gate/access-hook",
|
||||||
)
|
)
|
||||||
peer = self.client_address[0]
|
peer = self.client_address[0]
|
||||||
hook = subprocess.run(
|
try:
|
||||||
[hook_path, "upload-pack", str(repo_dir), peer, peer],
|
hook = subprocess.run(
|
||||||
capture_output=True,
|
[hook_path, "upload-pack", str(repo_dir), peer, peer],
|
||||||
check=False,
|
capture_output=True,
|
||||||
timeout=GIT_GATE_TIMEOUT_SECS,
|
check=False,
|
||||||
)
|
timeout=GIT_GATE_TIMEOUT_SECS,
|
||||||
|
)
|
||||||
|
except (OSError, subprocess.SubprocessError) as exc:
|
||||||
|
# The access-hook couldn't be run (missing, not executable,
|
||||||
|
# timed out, …). Fail closed with a real HTTP error rather
|
||||||
|
# than letting the exception kill the handler thread — an
|
||||||
|
# unhandled exception closes the socket with no response, which
|
||||||
|
# the client sees as an opaque "empty reply from server".
|
||||||
|
self.log_message(
|
||||||
|
"access-hook could not run for %s: %s", parsed.path, exc,
|
||||||
|
)
|
||||||
|
self.send_error(503, "git-gate access-hook unavailable")
|
||||||
|
return
|
||||||
if hook.returncode != 0:
|
if hook.returncode != 0:
|
||||||
detail = (hook.stderr or hook.stdout).decode(
|
detail = (hook.stderr or hook.stdout).decode(
|
||||||
"utf-8", errors="replace",
|
"utf-8", errors="replace",
|
||||||
|
|||||||
@@ -69,6 +69,18 @@ class TestProvisionGitGate(unittest.TestCase):
|
|||||||
self.assertEqual(1, len(exec_scripts))
|
self.assertEqual(1, len(exec_scripts))
|
||||||
self.assertIn("repo=/git/bottle1/${name}.git", exec_scripts[0][-1])
|
self.assertIn("repo=/git/bottle1/${name}.git", exec_scripts[0][-1])
|
||||||
|
|
||||||
|
def test_makes_access_hook_executable_on_the_gateway(self) -> None:
|
||||||
|
# Regression: the access-hook is exec'd directly, so it needs the x
|
||||||
|
# bit. The copy alone can't be trusted to carry the staged 0o700
|
||||||
|
# (`docker cp` preserves mode, the Apple `container cp` does not),
|
||||||
|
# so provisioning must re-apply +x on the gateway side.
|
||||||
|
calls: list[list[str]] = []
|
||||||
|
with patch(_RUN, side_effect=_recorder(calls)):
|
||||||
|
provision_git_gate(DockerGatewayTransport("gw"), "bottle1", _plan(_up("foo")))
|
||||||
|
self.assertIn(
|
||||||
|
["docker", "exec", "gw", "chmod", "+x", "/etc/git-gate/access-hook"], calls,
|
||||||
|
)
|
||||||
|
|
||||||
def test_omits_known_hosts_copy_when_absent(self) -> None:
|
def test_omits_known_hosts_copy_when_absent(self) -> None:
|
||||||
calls: list[list[str]] = []
|
calls: list[list[str]] = []
|
||||||
with patch(_RUN, side_effect=_recorder(calls)):
|
with patch(_RUN, side_effect=_recorder(calls)):
|
||||||
|
|||||||
@@ -364,6 +364,48 @@ class TestGitHttpBackend(unittest.TestCase):
|
|||||||
self.assertIn("access-hook denied", logged)
|
self.assertIn("access-hook denied", logged)
|
||||||
self.assertIn("exit=2", logged)
|
self.assertIn("exit=2", logged)
|
||||||
|
|
||||||
|
def test_access_hook_that_cannot_run_fails_closed_503(self):
|
||||||
|
"""Regression: when the access-hook can't be exec'd (missing / not
|
||||||
|
executable — a PermissionError from subprocess.run), the handler must
|
||||||
|
fail closed with a real HTTP status instead of letting the exception
|
||||||
|
kill the thread, which closes the socket with no response and the
|
||||||
|
client sees an opaque "empty reply from server"."""
|
||||||
|
from http.server import ThreadingHTTPServer
|
||||||
|
import io
|
||||||
|
import sys
|
||||||
|
|
||||||
|
with tempfile.TemporaryDirectory() as tmp:
|
||||||
|
root = Path(tmp)
|
||||||
|
(root / "repo.git").mkdir()
|
||||||
|
old_root = os.environ.get("GIT_PROJECT_ROOT")
|
||||||
|
os.environ["GIT_PROJECT_ROOT"] = str(root)
|
||||||
|
self.addCleanup(self._restore_env, old_root)
|
||||||
|
|
||||||
|
server = ThreadingHTTPServer(("127.0.0.1", 0), GitHttpHandler)
|
||||||
|
thread = threading.Thread(target=server.serve_forever, daemon=True)
|
||||||
|
thread.start()
|
||||||
|
self.addCleanup(server.shutdown)
|
||||||
|
self.addCleanup(server.server_close)
|
||||||
|
|
||||||
|
with mock.patch(
|
||||||
|
"bot_bottle.git_http_backend.subprocess.run",
|
||||||
|
side_effect=PermissionError(13, "Permission denied"),
|
||||||
|
):
|
||||||
|
buf = io.StringIO()
|
||||||
|
with mock.patch.object(sys, "stdout", buf):
|
||||||
|
req = urllib.request.Request(
|
||||||
|
f"http://127.0.0.1:{server.server_port}"
|
||||||
|
"/repo.git/info/refs?service=git-upload-pack",
|
||||||
|
method="GET",
|
||||||
|
)
|
||||||
|
try:
|
||||||
|
urllib.request.urlopen(req, timeout=5)
|
||||||
|
self.fail("expected HTTPError 503")
|
||||||
|
except urllib.error.HTTPError as e: # type: ignore
|
||||||
|
self.assertEqual(503, e.code)
|
||||||
|
|
||||||
|
self.assertIn("access-hook could not run", buf.getvalue())
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _restore_env(value: str | None) -> None:
|
def _restore_env(value: str | None) -> None:
|
||||||
if value is None:
|
if value is None:
|
||||||
|
|||||||
Reference in New Issue
Block a user