From fb64c50276d4831af88c44c44d4d21872c73f97f Mon Sep 17 00:00:00 2001 From: claude Date: Wed, 3 Jun 2026 15:28:23 +0000 Subject: [PATCH] docs(prd): address review feedback on PRD 0048 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename deploy_key → provisioned_key throughout (manifest key, dataclass names, internal field names, test descriptions) - Revocation failure at teardown now halts cleanup and propagates loudly; a stranded key is a security concern that must surface --- docs/prds/0048-ssh-deploy-key-provisioning.md | 92 ++++++++++--------- 1 file changed, 50 insertions(+), 42 deletions(-) diff --git a/docs/prds/0048-ssh-deploy-key-provisioning.md b/docs/prds/0048-ssh-deploy-key-provisioning.md index c14bcf7..faefe01 100644 --- a/docs/prds/0048-ssh-deploy-key-provisioning.md +++ b/docs/prds/0048-ssh-deploy-key-provisioning.md @@ -11,7 +11,7 @@ Replace per-repo static SSH identity files with short-lived ed25519 deploy keys that are generated at spin-up and revoked at teardown. Introduce `bot_bottle/contrib/` as the package for platform-specific provisioners and ship the first contrib sub-package: `bot_bottle/contrib/gitea/` with -`GiteaDeployKeyProvisioner`. A new `deploy_key:` block in `git-gate.repos` +`GiteaDeployKeyProvisioner`. A new `provisioned_key:` block in `git-gate.repos` entries opts a repo into automatic key lifecycle management; `identity:` stays valid for operators who supply their own key material. @@ -34,22 +34,22 @@ across every bottle spin-up. This has several consequences: ## Goals / Success Criteria -- `git-gate.repos` entries accept `deploy_key:` as an alternative to +- `git-gate.repos` entries accept `provisioned_key:` as an alternative to `identity:`. The parser rejects entries that have both, or neither. -- `deploy_key.provider: gitea` provisions and revokes deploy keys via the +- `provisioned_key.provider: gitea` provisions and revokes deploy keys via the Gitea HTTP API. - At prepare time the provisioner generates a fresh ed25519 keypair, registers the public half as a repo-scoped deploy key, and makes the private key available to git-gate at the path it expects — the rest of the pipeline is unchanged. - At teardown the provisioner deletes the registered deploy key. Failure to - delete is logged as a warning and does not abort teardown. + delete halts teardown and propagates the error loudly. - `bot_bottle/contrib/` is introduced as the package for platform-specific implementations; the core defines the abstract interface; contrib sub-packages provide concrete implementations. - Existing `identity:`-based repos continue to work without change. - The unit test suite passes unchanged for `identity:` paths; new tests cover - `deploy_key:` parse, validation, and provisioner dispatch. + `provisioned_key:` parse, validation, and provisioner dispatch. ## Non-goals @@ -76,27 +76,27 @@ host_key (optional string) After this PRD: ``` -url (required string) -identity (optional string — mutually exclusive with deploy_key) -deploy_key (optional object — mutually exclusive with identity) -host_key (optional string) +url (required string) +identity (optional string — mutually exclusive with provisioned_key) +provisioned_key (optional object — mutually exclusive with identity) +host_key (optional string) ``` -Exactly one of `identity` or `deploy_key` must be present. The parser emits a -targeted error for each violation: +Exactly one of `identity` or `provisioned_key` must be present. The parser +emits a targeted error for each violation: ``` bottle 'dev' git-gate.repos['bot-bottle'] must set exactly one of -'identity' or 'deploy_key'; got neither. +'identity' or 'provisioned_key'; got neither. bottle 'dev' git-gate.repos['bot-bottle'] must set exactly one of -'identity' or 'deploy_key'; got both. +'identity' or 'provisioned_key'; got both. ``` -`deploy_key` object schema: +`provisioned_key` object schema: ```yaml -deploy_key: +provisioned_key: provider: gitea # required; names the contrib module to load token_env: GITEA_TOKEN # required; name of a host env var holding the API token api_url: https://... # optional; defaults to https:// @@ -118,7 +118,7 @@ git-gate: repos: bot-bottle: url: ssh://git@gitea.dideric.is:30009/didericis/bot-bottle.git - deploy_key: + provisioned_key: provider: gitea token_env: GITEA_DEPLOY_TOKEN host_key: "ssh-rsa AAAA..." @@ -160,8 +160,10 @@ class DeployKeyProvisioner(ABC): @abstractmethod def delete(self, owner_repo: str, key_id: str) -> None: - """Delete the registered deploy key. Best-effort; should not - raise if the key is already absent.""" + """Delete the registered deploy key. + + Must not raise if the key is already absent (HTTP 404 is success). + Must raise for all other failures so that teardown halts.""" def get_provisioner(provider: str, token: str, api_url: str) -> DeployKeyProvisioner: @@ -175,7 +177,7 @@ def get_provisioner(provider: str, token: str, api_url: str) -> DeployKeyProvisi ) return GiteaDeployKeyProvisioner(token=token, api_url=api_url) from .manifest_util import ManifestError - raise ManifestError(f"unknown deploy_key provider: {provider!r}") + raise ManifestError(f"unknown provisioned_key provider: {provider!r}") ``` ### Gitea contrib implementation @@ -193,7 +195,8 @@ def get_provisioner(provider: str, token: str, api_url: str) -> DeployKeyProvisi `delete(owner_repo, key_id)`: 1. `DELETE /api/v1/repos/{owner}/{repo}/keys/{id}`. 2. Treat HTTP 404 as success (key already gone). -3. Log a warning and return (do not raise) for any other error. +3. Raise `RuntimeError` for any other non-2xx response or network error, + including the status code and response body in the message. HTTP calls use `urllib.request` from the stdlib; no new runtime dependency. @@ -201,22 +204,22 @@ HTTP calls use `urllib.request` from the stdlib; no new runtime dependency. `bot_bottle/manifest_git.py`: -- Add `DeployKeyConfig` dataclass: +- Add `ProvisionedKeyConfig` dataclass: ```python @dataclass(frozen=True) - class DeployKeyConfig: + class ProvisionedKeyConfig: provider: str token_env: str api_url: str # empty string means "derive from UpstreamHost" ``` - `GitEntry`: - - `IdentityFile: str` → `IdentityFile: str` (unchanged internally; empty string - when `deploy_key` is used; set at provision time, not parse time) - - New field: `DeployKey: DeployKeyConfig | None = None` + - `IdentityFile: str` unchanged internally; empty string when + `provisioned_key` is used; set at provision time, not parse time. + - New field: `ProvisionedKey: ProvisionedKeyConfig | None = None` - `from_repos_entry` validates the mutually-exclusive constraint and parses - the `deploy_key` block when present. + the `provisioned_key` block when present. ### `GitGateUpstream` / prepare-time changes @@ -226,13 +229,13 @@ The existing path writes the identity file path into `GitGateUpstream.IdentityFi and docker-cp's it into `/git-gate/creds/-key`. That path stays unchanged for `identity:` repos. -For `deploy_key:` repos, a new helper `provision_deploy_key(entry, stage_dir, -bottle_name)` runs before the git-gate sidecar starts: +For `provisioned_key:` repos, a new helper `provision_deploy_key(entry, +stage_dir, bottle_name)` runs before the git-gate sidecar starts: -1. Resolve `token = os.environ[entry.DeployKey.token_env]`. Missing key raises - `RuntimeError` with a clear message naming the env var. -2. Resolve `api_url = entry.DeployKey.api_url or f"https://{entry.UpstreamHost}"`. -3. Instantiate `get_provisioner(entry.DeployKey.provider, token, api_url)`. +1. Resolve `token = os.environ[entry.ProvisionedKey.token_env]`. Missing key + raises `RuntimeError` with a clear message naming the env var. +2. Resolve `api_url = entry.ProvisionedKey.api_url or f"https://{entry.UpstreamHost}"`. +3. Instantiate `get_provisioner(entry.ProvisionedKey.provider, token, api_url)`. 4. Call `provisioner.create(entry.UpstreamPath.lstrip("/"), title)` where `title = f"bot-bottle:{bottle_name}:{entry.Name}"`. 5. Write private key to `stage_dir / f"{entry.Name}-key"` (mode 0o600). @@ -246,13 +249,18 @@ bottle_name)` runs before the git-gate sidecar starts: `bot_bottle/backend/docker/cleanup.py` (or the equivalent teardown path): -After the git-gate sidecar stops, for each `GitEntry` with `DeployKey` set: +After the git-gate sidecar stops, for each `GitEntry` with `ProvisionedKey` +set: -1. Check that `stage_dir / f"{entry.Name}-deploy-key-id"` exists; skip if absent - (provision never ran or already cleaned up). +1. Check that `stage_dir / f"{entry.Name}-deploy-key-id"` exists; skip if + absent (provision never ran or already cleaned up). 2. Resolve token and API URL as above. 3. Instantiate provisioner and call `provisioner.delete(owner_repo, key_id)`. -4. Log result at INFO (success) or WARNING (failure); continue regardless. +4. On success, log at INFO. On failure, allow the exception to propagate — + teardown halts and the error surfaces to the operator. + +A stranded deploy key is a security concern: the operator must know about it +and address it manually. Silent continuation is not acceptable. The private key file in `stage_dir` is cleaned up as part of normal stage-dir teardown (no extra step needed). @@ -266,11 +274,11 @@ python3 -m unittest discover -s tests/unit New / modified test files: - `tests/unit/test_manifest_git.py` — add cases for: - - `deploy_key:` accepted with valid `provider`, `token_env`, optional `api_url` - - Both `identity` and `deploy_key` present → `ManifestError` - - Neither `identity` nor `deploy_key` present → `ManifestError` - - Unknown key inside `deploy_key` block → `ManifestError` - - Missing `provider` or `token_env` inside `deploy_key` → `ManifestError` + - `provisioned_key:` accepted with valid `provider`, `token_env`, optional `api_url` + - Both `identity` and `provisioned_key` present → `ManifestError` + - Neither `identity` nor `provisioned_key` present → `ManifestError` + - Unknown key inside `provisioned_key` block → `ManifestError` + - Missing `provider` or `token_env` inside `provisioned_key` → `ManifestError` - `tests/unit/test_deploy_key_provisioner.py` — new: - `get_provisioner("gitea", ...)` returns `GiteaDeployKeyProvisioner` @@ -281,7 +289,7 @@ New / modified test files: - `create()` calls `ssh-keygen`, POSTs to correct endpoint, returns key ID - `delete()` DELETEs to correct endpoint - `delete()` tolerates HTTP 404 (already-deleted key) - - `delete()` logs warning (does not raise) on other HTTP errors + - `delete()` raises `RuntimeError` on non-404 HTTP error ## Open questions