fix: add explicit timeouts to subprocess and HTTP calls in git-gate paths
Closes #255. Without timeouts, a hung upstream during the access-hook or git http-backend CGI call (git_http_backend.py) and a stalled Gitea API during deploy-key provisioning (contrib/gitea/deploy_key_provisioner.py) could wedge a sidecar indefinitely. Adds GIT_HTTP_BACKEND_TIMEOUT_SECS (30s) to both subprocess.run calls in the HTTP backend, mirroring the existing GIT_GATE_DAEMON_TIMEOUT_SECS on the daemon path. Adds _API_TIMEOUT_SECS (30s) and _KEYGEN_TIMEOUT_SECS (10s) to the Gitea provisioner's urlopen and ssh-keygen calls. Tests verify the timeout values are forwarded in all four call sites.
This commit is contained in:
@@ -21,6 +21,11 @@ from pathlib import Path
|
|||||||
|
|
||||||
from ...deploy_key_provisioner import DeployKeyCollisionError, DeployKeyProvisioner
|
from ...deploy_key_provisioner import DeployKeyCollisionError, DeployKeyProvisioner
|
||||||
|
|
||||||
|
# Timeout for ssh-keygen and Gitea API HTTP calls. A hung Gitea instance at
|
||||||
|
# prepare time would stall bottle launch indefinitely without this bound.
|
||||||
|
_API_TIMEOUT_SECS = 30
|
||||||
|
_KEYGEN_TIMEOUT_SECS = 10
|
||||||
|
|
||||||
|
|
||||||
class GiteaDeployKeyProvisioner(DeployKeyProvisioner):
|
class GiteaDeployKeyProvisioner(DeployKeyProvisioner):
|
||||||
"""Manages deploy keys on a Gitea instance."""
|
"""Manages deploy keys on a Gitea instance."""
|
||||||
@@ -46,6 +51,7 @@ class GiteaDeployKeyProvisioner(DeployKeyProvisioner):
|
|||||||
check=True,
|
check=True,
|
||||||
stdout=subprocess.DEVNULL,
|
stdout=subprocess.DEVNULL,
|
||||||
stderr=subprocess.DEVNULL,
|
stderr=subprocess.DEVNULL,
|
||||||
|
timeout=_KEYGEN_TIMEOUT_SECS,
|
||||||
)
|
)
|
||||||
private_key = key_path.read_bytes()
|
private_key = key_path.read_bytes()
|
||||||
public_key = key_path.with_suffix(".pub").read_text().strip()
|
public_key = key_path.with_suffix(".pub").read_text().strip()
|
||||||
@@ -67,7 +73,7 @@ class GiteaDeployKeyProvisioner(DeployKeyProvisioner):
|
|||||||
method="POST",
|
method="POST",
|
||||||
)
|
)
|
||||||
try:
|
try:
|
||||||
with urllib.request.urlopen(req) as resp:
|
with urllib.request.urlopen(req, timeout=_API_TIMEOUT_SECS) as resp:
|
||||||
body = json.loads(resp.read())
|
body = json.loads(resp.read())
|
||||||
except urllib.error.HTTPError as exc:
|
except urllib.error.HTTPError as exc:
|
||||||
_body = _read_error_body(exc)
|
_body = _read_error_body(exc)
|
||||||
@@ -98,7 +104,7 @@ class GiteaDeployKeyProvisioner(DeployKeyProvisioner):
|
|||||||
method="DELETE",
|
method="DELETE",
|
||||||
)
|
)
|
||||||
try:
|
try:
|
||||||
with urllib.request.urlopen(req):
|
with urllib.request.urlopen(req, timeout=_API_TIMEOUT_SECS):
|
||||||
pass
|
pass
|
||||||
except urllib.error.HTTPError as exc:
|
except urllib.error.HTTPError as exc:
|
||||||
if exc.code == 404:
|
if exc.code == 404:
|
||||||
|
|||||||
@@ -22,6 +22,12 @@ DEFAULT_PORT = 9420
|
|||||||
# Bound memory use while still allowing ordinary git push packfiles.
|
# Bound memory use while still allowing ordinary git push packfiles.
|
||||||
MAX_BODY_BYTES = 100 * 1024 * 1024
|
MAX_BODY_BYTES = 100 * 1024 * 1024
|
||||||
|
|
||||||
|
# Timeout for the access-hook subprocess and git http-backend CGI subprocess.
|
||||||
|
# Mirrors GIT_GATE_DAEMON_TIMEOUT_SECS so both HTTP and daemon paths share the
|
||||||
|
# same bound: a hung upstream fetch in the access-hook or a stalled CGI child
|
||||||
|
# cannot wedge the sidecar indefinitely.
|
||||||
|
GIT_HTTP_BACKEND_TIMEOUT_SECS = 30
|
||||||
|
|
||||||
|
|
||||||
class GitHttpHandler(BaseHTTPRequestHandler):
|
class GitHttpHandler(BaseHTTPRequestHandler):
|
||||||
server_version = "bot-bottle-git-http/1"
|
server_version = "bot-bottle-git-http/1"
|
||||||
@@ -47,6 +53,7 @@ class GitHttpHandler(BaseHTTPRequestHandler):
|
|||||||
[hook_path, "upload-pack", str(repo_dir), peer, peer],
|
[hook_path, "upload-pack", str(repo_dir), peer, peer],
|
||||||
capture_output=True,
|
capture_output=True,
|
||||||
check=False,
|
check=False,
|
||||||
|
timeout=GIT_HTTP_BACKEND_TIMEOUT_SECS,
|
||||||
)
|
)
|
||||||
if hook.returncode != 0:
|
if hook.returncode != 0:
|
||||||
detail = (hook.stderr or hook.stdout).decode(
|
detail = (hook.stderr or hook.stdout).decode(
|
||||||
@@ -110,6 +117,7 @@ class GitHttpHandler(BaseHTTPRequestHandler):
|
|||||||
env=env,
|
env=env,
|
||||||
capture_output=True,
|
capture_output=True,
|
||||||
check=False,
|
check=False,
|
||||||
|
timeout=GIT_HTTP_BACKEND_TIMEOUT_SECS,
|
||||||
)
|
)
|
||||||
self._write_cgi_response(proc.stdout)
|
self._write_cgi_response(proc.stdout)
|
||||||
|
|
||||||
|
|||||||
@@ -10,6 +10,8 @@ from unittest.mock import MagicMock, patch
|
|||||||
|
|
||||||
from bot_bottle.contrib.gitea.deploy_key_provisioner import (
|
from bot_bottle.contrib.gitea.deploy_key_provisioner import (
|
||||||
GiteaDeployKeyProvisioner,
|
GiteaDeployKeyProvisioner,
|
||||||
|
_API_TIMEOUT_SECS,
|
||||||
|
_KEYGEN_TIMEOUT_SECS,
|
||||||
_split_owner_repo,
|
_split_owner_repo,
|
||||||
)
|
)
|
||||||
from bot_bottle.deploy_key_provisioner import DeployKeyCollisionError
|
from bot_bottle.deploy_key_provisioner import DeployKeyCollisionError
|
||||||
@@ -83,6 +85,25 @@ class TestCreate(unittest.TestCase):
|
|||||||
self.assertEqual(str(fake_key_id), key_id)
|
self.assertEqual(str(fake_key_id), key_id)
|
||||||
self.assertEqual(fake_private, private_bytes)
|
self.assertEqual(fake_private, private_bytes)
|
||||||
|
|
||||||
|
def test_create_passes_timeout_to_ssh_keygen_and_urlopen(self):
|
||||||
|
provisioner = _provisioner()
|
||||||
|
with patch(
|
||||||
|
"bot_bottle.contrib.gitea.deploy_key_provisioner.subprocess.run"
|
||||||
|
) as mock_run, patch(
|
||||||
|
"bot_bottle.contrib.gitea.deploy_key_provisioner.urllib.request.urlopen"
|
||||||
|
) as mock_urlopen, patch(
|
||||||
|
"bot_bottle.contrib.gitea.deploy_key_provisioner.Path.read_bytes",
|
||||||
|
return_value=b"PRIVATE",
|
||||||
|
), patch(
|
||||||
|
"bot_bottle.contrib.gitea.deploy_key_provisioner.Path.read_text",
|
||||||
|
return_value="ssh-ed25519 AAAA\n",
|
||||||
|
):
|
||||||
|
mock_urlopen.return_value = _urlopen_response({"id": 1})
|
||||||
|
provisioner.create("owner/repo", "title")
|
||||||
|
|
||||||
|
self.assertEqual(_KEYGEN_TIMEOUT_SECS, mock_run.call_args.kwargs.get("timeout"))
|
||||||
|
self.assertEqual(_API_TIMEOUT_SECS, mock_urlopen.call_args.kwargs.get("timeout"))
|
||||||
|
|
||||||
def test_create_raises_on_http_error(self):
|
def test_create_raises_on_http_error(self):
|
||||||
provisioner = _provisioner()
|
provisioner = _provisioner()
|
||||||
with patch(
|
with patch(
|
||||||
@@ -139,6 +160,16 @@ class TestDelete(unittest.TestCase):
|
|||||||
self.assertIn("/api/v1/repos/didericis/bot-bottle/keys/99", req.full_url)
|
self.assertIn("/api/v1/repos/didericis/bot-bottle/keys/99", req.full_url)
|
||||||
self.assertEqual("DELETE", req.get_method())
|
self.assertEqual("DELETE", req.get_method())
|
||||||
|
|
||||||
|
def test_delete_passes_timeout_to_urlopen(self):
|
||||||
|
provisioner = _provisioner()
|
||||||
|
with patch(
|
||||||
|
"bot_bottle.contrib.gitea.deploy_key_provisioner.urllib.request.urlopen"
|
||||||
|
) as mock_urlopen:
|
||||||
|
mock_urlopen.return_value = _urlopen_response({})
|
||||||
|
provisioner.delete("owner/repo", "7")
|
||||||
|
|
||||||
|
self.assertEqual(_API_TIMEOUT_SECS, mock_urlopen.call_args.kwargs.get("timeout"))
|
||||||
|
|
||||||
def test_delete_tolerates_404(self):
|
def test_delete_tolerates_404(self):
|
||||||
provisioner = _provisioner()
|
provisioner = _provisioner()
|
||||||
with patch(
|
with patch(
|
||||||
|
|||||||
@@ -9,7 +9,11 @@ import urllib.request
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from unittest import mock
|
from unittest import mock
|
||||||
|
|
||||||
from bot_bottle.git_http_backend import GitHttpHandler, MAX_BODY_BYTES
|
from bot_bottle.git_http_backend import (
|
||||||
|
GIT_HTTP_BACKEND_TIMEOUT_SECS,
|
||||||
|
GitHttpHandler,
|
||||||
|
MAX_BODY_BYTES,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class TestGitHttpBackend(unittest.TestCase):
|
class TestGitHttpBackend(unittest.TestCase):
|
||||||
@@ -150,6 +154,61 @@ class TestGitHttpBackend(unittest.TestCase):
|
|||||||
)
|
)
|
||||||
self.assertEqual("git/test", env["HTTP_USER_AGENT"])
|
self.assertEqual("git/test", env["HTTP_USER_AGENT"])
|
||||||
|
|
||||||
|
def test_subprocess_calls_include_timeout(self):
|
||||||
|
"""Both subprocess.run calls (access-hook and git http-backend) must
|
||||||
|
pass timeout= so a hung upstream cannot wedge the sidecar."""
|
||||||
|
from http.server import ThreadingHTTPServer
|
||||||
|
|
||||||
|
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)
|
||||||
|
old_hook = os.environ.get("GIT_GATE_ACCESS_HOOK")
|
||||||
|
hook = root / "access-hook"
|
||||||
|
hook.write_text("#!/bin/sh\nexit 0\n")
|
||||||
|
hook.chmod(0o700)
|
||||||
|
os.environ["GIT_GATE_ACCESS_HOOK"] = str(hook)
|
||||||
|
self.addCleanup(self._restore_hook, old_hook)
|
||||||
|
|
||||||
|
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)
|
||||||
|
|
||||||
|
backend_response = (
|
||||||
|
b"Status: 200 OK\r\n"
|
||||||
|
b"Content-Type: application/x-git-upload-pack-result\r\n"
|
||||||
|
b"\r\n"
|
||||||
|
b"0000"
|
||||||
|
)
|
||||||
|
calls = [
|
||||||
|
subprocess.CompletedProcess(["hook"], 0, b"", b""),
|
||||||
|
subprocess.CompletedProcess(["git"], 0, backend_response, b""),
|
||||||
|
]
|
||||||
|
with mock.patch(
|
||||||
|
"bot_bottle.git_http_backend.subprocess.run",
|
||||||
|
side_effect=calls,
|
||||||
|
) as run:
|
||||||
|
req = urllib.request.Request(
|
||||||
|
f"http://127.0.0.1:{server.server_port}"
|
||||||
|
"/repo.git/git-upload-pack",
|
||||||
|
data=b"",
|
||||||
|
method="POST",
|
||||||
|
)
|
||||||
|
with urllib.request.urlopen(req, timeout=5):
|
||||||
|
pass
|
||||||
|
|
||||||
|
for call in run.call_args_list:
|
||||||
|
self.assertEqual(
|
||||||
|
GIT_HTTP_BACKEND_TIMEOUT_SECS,
|
||||||
|
call.kwargs.get("timeout"),
|
||||||
|
f"subprocess.run call missing timeout: {call}",
|
||||||
|
)
|
||||||
|
|
||||||
def test_access_hook_denial_is_logged_to_stdout(self):
|
def test_access_hook_denial_is_logged_to_stdout(self):
|
||||||
"""When the access-hook exits non-zero we still return 403 to the
|
"""When the access-hook exits non-zero we still return 403 to the
|
||||||
client, but the hook's stderr must also appear on the handler's
|
client, but the hook's stderr must also appear on the handler's
|
||||||
|
|||||||
Reference in New Issue
Block a user