docs(prd): address review feedback on PRD 0048
- 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
This commit is contained in:
@@ -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
|
keys that are generated at spin-up and revoked at teardown. Introduce
|
||||||
`bot_bottle/contrib/` as the package for platform-specific provisioners and
|
`bot_bottle/contrib/` as the package for platform-specific provisioners and
|
||||||
ship the first contrib sub-package: `bot_bottle/contrib/gitea/` with
|
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
|
entries opts a repo into automatic key lifecycle management; `identity:` stays
|
||||||
valid for operators who supply their own key material.
|
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
|
## 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.
|
`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.
|
Gitea HTTP API.
|
||||||
- At prepare time the provisioner generates a fresh ed25519 keypair, registers
|
- 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
|
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
|
available to git-gate at the path it expects — the rest of the pipeline is
|
||||||
unchanged.
|
unchanged.
|
||||||
- At teardown the provisioner deletes the registered deploy key. Failure to
|
- 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
|
- `bot_bottle/contrib/` is introduced as the package for platform-specific
|
||||||
implementations; the core defines the abstract interface; contrib sub-packages
|
implementations; the core defines the abstract interface; contrib sub-packages
|
||||||
provide concrete implementations.
|
provide concrete implementations.
|
||||||
- Existing `identity:`-based repos continue to work without change.
|
- Existing `identity:`-based repos continue to work without change.
|
||||||
- The unit test suite passes unchanged for `identity:` paths; new tests cover
|
- 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
|
## Non-goals
|
||||||
|
|
||||||
@@ -76,27 +76,27 @@ host_key (optional string)
|
|||||||
After this PRD:
|
After this PRD:
|
||||||
|
|
||||||
```
|
```
|
||||||
url (required string)
|
url (required string)
|
||||||
identity (optional string — mutually exclusive with deploy_key)
|
identity (optional string — mutually exclusive with provisioned_key)
|
||||||
deploy_key (optional object — mutually exclusive with identity)
|
provisioned_key (optional object — mutually exclusive with identity)
|
||||||
host_key (optional string)
|
host_key (optional string)
|
||||||
```
|
```
|
||||||
|
|
||||||
Exactly one of `identity` or `deploy_key` must be present. The parser emits a
|
Exactly one of `identity` or `provisioned_key` must be present. The parser
|
||||||
targeted error for each violation:
|
emits a targeted error for each violation:
|
||||||
|
|
||||||
```
|
```
|
||||||
bottle 'dev' git-gate.repos['bot-bottle'] must set exactly one of
|
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
|
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
|
```yaml
|
||||||
deploy_key:
|
provisioned_key:
|
||||||
provider: gitea # required; names the contrib module to load
|
provider: gitea # required; names the contrib module to load
|
||||||
token_env: GITEA_TOKEN # required; name of a host env var holding the API token
|
token_env: GITEA_TOKEN # required; name of a host env var holding the API token
|
||||||
api_url: https://... # optional; defaults to https://<host from url>
|
api_url: https://... # optional; defaults to https://<host from url>
|
||||||
@@ -118,7 +118,7 @@ git-gate:
|
|||||||
repos:
|
repos:
|
||||||
bot-bottle:
|
bot-bottle:
|
||||||
url: ssh://git@gitea.dideric.is:30009/didericis/bot-bottle.git
|
url: ssh://git@gitea.dideric.is:30009/didericis/bot-bottle.git
|
||||||
deploy_key:
|
provisioned_key:
|
||||||
provider: gitea
|
provider: gitea
|
||||||
token_env: GITEA_DEPLOY_TOKEN
|
token_env: GITEA_DEPLOY_TOKEN
|
||||||
host_key: "ssh-rsa AAAA..."
|
host_key: "ssh-rsa AAAA..."
|
||||||
@@ -160,8 +160,10 @@ class DeployKeyProvisioner(ABC):
|
|||||||
|
|
||||||
@abstractmethod
|
@abstractmethod
|
||||||
def delete(self, owner_repo: str, key_id: str) -> None:
|
def delete(self, owner_repo: str, key_id: str) -> None:
|
||||||
"""Delete the registered deploy key. Best-effort; should not
|
"""Delete the registered deploy key.
|
||||||
raise if the key is already absent."""
|
|
||||||
|
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:
|
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)
|
return GiteaDeployKeyProvisioner(token=token, api_url=api_url)
|
||||||
from .manifest_util import ManifestError
|
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
|
### Gitea contrib implementation
|
||||||
@@ -193,7 +195,8 @@ def get_provisioner(provider: str, token: str, api_url: str) -> DeployKeyProvisi
|
|||||||
`delete(owner_repo, key_id)`:
|
`delete(owner_repo, key_id)`:
|
||||||
1. `DELETE /api/v1/repos/{owner}/{repo}/keys/{id}`.
|
1. `DELETE /api/v1/repos/{owner}/{repo}/keys/{id}`.
|
||||||
2. Treat HTTP 404 as success (key already gone).
|
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.
|
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`:
|
`bot_bottle/manifest_git.py`:
|
||||||
|
|
||||||
- Add `DeployKeyConfig` dataclass:
|
- Add `ProvisionedKeyConfig` dataclass:
|
||||||
|
|
||||||
```python
|
```python
|
||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
class DeployKeyConfig:
|
class ProvisionedKeyConfig:
|
||||||
provider: str
|
provider: str
|
||||||
token_env: str
|
token_env: str
|
||||||
api_url: str # empty string means "derive from UpstreamHost"
|
api_url: str # empty string means "derive from UpstreamHost"
|
||||||
```
|
```
|
||||||
|
|
||||||
- `GitEntry`:
|
- `GitEntry`:
|
||||||
- `IdentityFile: str` → `IdentityFile: str` (unchanged internally; empty string
|
- `IdentityFile: str` unchanged internally; empty string when
|
||||||
when `deploy_key` is used; set at provision time, not parse time)
|
`provisioned_key` is used; set at provision time, not parse time.
|
||||||
- New field: `DeployKey: DeployKeyConfig | None = None`
|
- New field: `ProvisionedKey: ProvisionedKeyConfig | None = None`
|
||||||
- `from_repos_entry` validates the mutually-exclusive constraint and parses
|
- `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
|
### `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/<name>-key`. That path stays unchanged
|
and docker-cp's it into `/git-gate/creds/<name>-key`. That path stays unchanged
|
||||||
for `identity:` repos.
|
for `identity:` repos.
|
||||||
|
|
||||||
For `deploy_key:` repos, a new helper `provision_deploy_key(entry, stage_dir,
|
For `provisioned_key:` repos, a new helper `provision_deploy_key(entry,
|
||||||
bottle_name)` runs before the git-gate sidecar starts:
|
stage_dir, bottle_name)` runs before the git-gate sidecar starts:
|
||||||
|
|
||||||
1. Resolve `token = os.environ[entry.DeployKey.token_env]`. Missing key raises
|
1. Resolve `token = os.environ[entry.ProvisionedKey.token_env]`. Missing key
|
||||||
`RuntimeError` with a clear message naming the env var.
|
raises `RuntimeError` with a clear message naming the env var.
|
||||||
2. Resolve `api_url = entry.DeployKey.api_url or f"https://{entry.UpstreamHost}"`.
|
2. Resolve `api_url = entry.ProvisionedKey.api_url or f"https://{entry.UpstreamHost}"`.
|
||||||
3. Instantiate `get_provisioner(entry.DeployKey.provider, token, api_url)`.
|
3. Instantiate `get_provisioner(entry.ProvisionedKey.provider, token, api_url)`.
|
||||||
4. Call `provisioner.create(entry.UpstreamPath.lstrip("/"), title)` where
|
4. Call `provisioner.create(entry.UpstreamPath.lstrip("/"), title)` where
|
||||||
`title = f"bot-bottle:{bottle_name}:{entry.Name}"`.
|
`title = f"bot-bottle:{bottle_name}:{entry.Name}"`.
|
||||||
5. Write private key to `stage_dir / f"{entry.Name}-key"` (mode 0o600).
|
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):
|
`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
|
1. Check that `stage_dir / f"{entry.Name}-deploy-key-id"` exists; skip if
|
||||||
(provision never ran or already cleaned up).
|
absent (provision never ran or already cleaned up).
|
||||||
2. Resolve token and API URL as above.
|
2. Resolve token and API URL as above.
|
||||||
3. Instantiate provisioner and call `provisioner.delete(owner_repo, key_id)`.
|
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
|
The private key file in `stage_dir` is cleaned up as part of normal stage-dir
|
||||||
teardown (no extra step needed).
|
teardown (no extra step needed).
|
||||||
@@ -266,11 +274,11 @@ python3 -m unittest discover -s tests/unit
|
|||||||
New / modified test files:
|
New / modified test files:
|
||||||
|
|
||||||
- `tests/unit/test_manifest_git.py` — add cases for:
|
- `tests/unit/test_manifest_git.py` — add cases for:
|
||||||
- `deploy_key:` accepted with valid `provider`, `token_env`, optional `api_url`
|
- `provisioned_key:` accepted with valid `provider`, `token_env`, optional `api_url`
|
||||||
- Both `identity` and `deploy_key` present → `ManifestError`
|
- Both `identity` and `provisioned_key` present → `ManifestError`
|
||||||
- Neither `identity` nor `deploy_key` present → `ManifestError`
|
- Neither `identity` nor `provisioned_key` present → `ManifestError`
|
||||||
- Unknown key inside `deploy_key` block → `ManifestError`
|
- Unknown key inside `provisioned_key` block → `ManifestError`
|
||||||
- Missing `provider` or `token_env` inside `deploy_key` → `ManifestError`
|
- Missing `provider` or `token_env` inside `provisioned_key` → `ManifestError`
|
||||||
|
|
||||||
- `tests/unit/test_deploy_key_provisioner.py` — new:
|
- `tests/unit/test_deploy_key_provisioner.py` — new:
|
||||||
- `get_provisioner("gitea", ...)` returns `GiteaDeployKeyProvisioner`
|
- `get_provisioner("gitea", ...)` returns `GiteaDeployKeyProvisioner`
|
||||||
@@ -281,7 +289,7 @@ New / modified test files:
|
|||||||
- `create()` calls `ssh-keygen`, POSTs to correct endpoint, returns key ID
|
- `create()` calls `ssh-keygen`, POSTs to correct endpoint, returns key ID
|
||||||
- `delete()` DELETEs to correct endpoint
|
- `delete()` DELETEs to correct endpoint
|
||||||
- `delete()` tolerates HTTP 404 (already-deleted key)
|
- `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
|
## Open questions
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user