phase 13.x.7: encryption admin CLI + rotate-key endpoint#23
Merged
Conversation
POST /v1/admin/encryption/rotate-key generates a fresh age keypair via the Task 1 keygen primitive, writes it under the conventional PHOENIX_ENCRYPTION_KEYS_DIR, and returns paths + 16-hex fingerprints. Permission-gated on both is_admin (standard admin chain) and the Task 3 can_rotate_encryption_key flag (defense in depth). Body fields: name (default 'rotation-<date>') -- slug for the keypair files force (default false) -- overwrite vs 409 on conflict Response includes next_step copy explaining the daemon-restart requirement: existing data stays decryptable with the prior identity and new encrypts pick up both recipients per encryption_age's multi-recipient lossless-rotation design. Audit events emit recipient_fingerprint + recipient_path on success and per-error variants on each failure path; never the identity secret. Tests cover happy path, default name, 409 conflict, force override, and the non-admin 403 path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements a shared age keypair generation primitive, exposes it via a new Sequence diagram for POST /v1/admin/encryption/rotate-keysequenceDiagram
actor Operator
participant AdminAPI as rotate_encryption_key
participant Authn as _admin_authn
participant Identity as extract_or_bootstrap
participant Safety as verify_request
participant Perms as get_permissions_registry
participant Keygen as generate_age_keypair
participant Audit as emit_admin_audit
Operator->>AdminAPI: POST /v1/admin/encryption/rotate-key
AdminAPI->>Authn: _admin_authn(request, authorization)
Authn->>Identity: extract_or_bootstrap(authorization)
Identity-->>Authn: actor
Authn->>Safety: verify_request(actor, action_key="admin.mutate")
Safety-->>Authn: ok
Authn->>Perms: get_permissions_registry().get(actor.name)
Perms-->>Authn: perms (can_rotate_encryption_key)
Authn-->>AdminAPI: actor
AdminAPI->>Keygen: generate_age_keypair(keys_dir=None, name, force)
Keygen-->>AdminAPI: GeneratedKeyPair
AdminAPI->>Audit: emit_admin_audit(event_type="admin.encryption.rotate.success")
AdminAPI-->>Operator: 200 OK {paths, fingerprints, next_step}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
_validate_nameerror message inphoenix/ledger/keygen.pymentions only leading '.' or '-' as disallowed, but the regex actually forbids any non-alphanumeric first character (including '_'); consider tightening the message or relaxing the regex so behavior and text match. - In
_admin_authn(encryption_admin),get_permissions_registry().get(actor.name)is used without a fallback; it might be safer to handle a missing registry entry explicitly (e.g., defaulting to deny or raising a clearer error) before accessingcan_rotate_encryption_key. - The fake
pyrageIdentity/Recipient helpers are duplicated across multiple test files; extracting a shared test helper module would reduce repetition and keep future changes to the stubbed crypto surface in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_validate_name` error message in `phoenix/ledger/keygen.py` mentions only leading '.' or '-' as disallowed, but the regex actually forbids any non-alphanumeric first character (including '_'); consider tightening the message or relaxing the regex so behavior and text match.
- In `_admin_authn` (encryption_admin), `get_permissions_registry().get(actor.name)` is used without a fallback; it might be safer to handle a missing registry entry explicitly (e.g., defaulting to deny or raising a clearer error) before accessing `can_rotate_encryption_key`.
- The fake `pyrage` Identity/Recipient helpers are duplicated across multiple test files; extracting a shared test helper module would reduce repetition and keep future changes to the stubbed crypto surface in one place.
## Individual Comments
### Comment 1
<location path="tests/integration/test_admin_encryption_rotate_key.py" line_range="137-144" />
<code_context>
+# Tests.
+
+
+class TestRotateKeyEndpoint:
+ def test_happy_path_returns_200(
+ self,
+ fake_pyrage: Any,
+ isolated_runtime: Path,
+ ) -> None:
+ keys_dir = Path(os.environ["PHOENIX_ENCRYPTION_KEYS_DIR"])
+ with TestClient(app) as client:
+ resp = client.post(
+ "/v1/admin/encryption/rotate-key",
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for actors with is_admin=True but can_rotate_encryption_key=False
Since the implementation performs a second permission check after `require_admin`, this scenario should be covered by an integration test that uses an admin actor with `can_rotate_encryption_key=False`, calls `POST /v1/admin/encryption/rotate-key`, and asserts a 403 with an appropriate missing-permission message. That will verify that admin status alone is not enough to rotate keys and guard against regressions in the authorization logic.
Suggested implementation:
```python
class TestRotateKeyEndpoint:
def test_happy_path_returns_200(
self,
fake_pyrage: Any,
isolated_runtime: Path,
) -> None:
keys_dir = Path(os.environ["PHOENIX_ENCRYPTION_KEYS_DIR"])
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 200, resp.text
body = resp.json()
# Paths land in the configured keys dir under the expected layout.
assert body["identity_path"].endswith("identity-rotation-2026-05-28.txt")
assert body["recipient_path"].endswith("rotation-2026-05-28.pub")
# Fingerprints are 16 lowercase hex chars and match each other
# (keygen primitive uses the recipient text for both sides).
assert len(body["identity_fingerprint"]) == 16
def test_admin_without_rotate_permission_gets_403(
self,
fake_pyrage: Any,
isolated_runtime: Path,
) -> None:
# Create an admin actor that is not allowed to rotate encryption keys.
actor = mint_bootstrap_actor("admin-no-rotate")
actor.is_admin = True
actor.can_rotate_encryption_key = False
payload = actor.to_payload()
auth_header = "Phoenix-Actor " + base64.b64encode(
json.dumps(payload).encode()
).decode("ascii")
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
headers={"Authorization": auth_header},
)
assert resp.status_code == 403, resp.text
body = resp.json()
# Admin status alone is not sufficient; missing rotate permission is enforced.
assert "can_rotate_encryption_key" in body.get("detail", "")
```
If the test suite uses a shared helper to construct the `Authorization` header from an actor (e.g. a function wrapping the `mint_bootstrap_actor(...).to_payload()` + base64 logic), replace the inline header construction in `test_admin_without_rotate_permission_gets_403` with that helper to match existing conventions.
If the error response format differs (for example, `{"error": {"code": "...", "message": "..."}}` or a different key than `"detail"`), update the final assertion to check the correct field and structure, keeping the core intent: asserting a 403 status and that the message indicates the missing `can_rotate_encryption_key` permission.
</issue_to_address>
### Comment 2
<location path="tests/integration/test_admin_encryption_rotate_key.py" line_range="138-147" />
<code_context>
+ def test_happy_path_returns_200(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test for keygen failure (KeyGenError/ImportError) returning 500 from the endpoint
To fully cover the endpoint’s documented behaviour, add an integration test where `generate_age_keypair` fails. For example, monkeypatch `phoenix.admin.encryption_admin.generate_age_keypair` to:
- Raise `KeyGenError` and assert a 500 with the expected "keygen failed" detail
- (Optionally) raise `ImportError` and assert a 500 with the import error detail
This will verify the error-to-HTTP 500 mapping and make the failure modes reliable for clients.
Suggested implementation:
```python
from phoenix.admin.encryption_admin import KeyGenError
class TestRotateKeyEndpoint:
```
```python
def test_happy_path_returns_200(
self,
fake_pyrage: Any,
isolated_runtime: Path,
) -> None:
keys_dir = Path(os.environ["PHOENIX_ENCRYPTION_KEYS_DIR"])
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 200
def test_keygen_error_returns_500(
self,
fake_pyrage: Any,
monkeypatch,
isolated_runtime: Path,
) -> None:
def _raise_keygen_error(*args, **kwargs):
raise KeyGenError("keygen failed")
import phoenix.admin.encryption_admin as encryption_admin
monkeypatch.setattr(
encryption_admin,
"generate_age_keypair",
_raise_keygen_error,
)
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 500
body = resp.json()
assert body["detail"] == "keygen failed"
def test_import_error_returns_500(
self,
fake_pyrage: Any,
monkeypatch,
isolated_runtime: Path,
) -> None:
def _raise_import_error(*args, **kwargs):
raise ImportError("age backend not available")
import phoenix.admin.encryption_admin as encryption_admin
monkeypatch.setattr(
encryption_admin,
"generate_age_keypair",
_raise_import_error,
)
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 500
body = resp.json()
assert "age backend not available" in body["detail"]
```
1. If `KeyGenError` is defined in a different module than `phoenix.admin.encryption_admin`, adjust the import line accordingly (e.g. `from phoenix.encryption import KeyGenError`).
2. If the endpoint wraps error messages differently (for example, prefixes them or nests them under another field), tweak the assertions on `body["detail"]` to match the actual response shape.
3. If the project uses a different fully-qualified path for `generate_age_keypair`, update the `import phoenix.admin.encryption_admin as encryption_admin` accordingly so the `monkeypatch.setattr` targets the correct function.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+137
to
+144
| class TestRotateKeyEndpoint: | ||
| def test_happy_path_returns_200( | ||
| self, | ||
| fake_pyrage: Any, | ||
| isolated_runtime: Path, | ||
| ) -> None: | ||
| keys_dir = Path(os.environ["PHOENIX_ENCRYPTION_KEYS_DIR"]) | ||
| with TestClient(app) as client: |
There was a problem hiding this comment.
suggestion (testing): Add a test for actors with is_admin=True but can_rotate_encryption_key=False
Since the implementation performs a second permission check after require_admin, this scenario should be covered by an integration test that uses an admin actor with can_rotate_encryption_key=False, calls POST /v1/admin/encryption/rotate-key, and asserts a 403 with an appropriate missing-permission message. That will verify that admin status alone is not enough to rotate keys and guard against regressions in the authorization logic.
Suggested implementation:
class TestRotateKeyEndpoint:
def test_happy_path_returns_200(
self,
fake_pyrage: Any,
isolated_runtime: Path,
) -> None:
keys_dir = Path(os.environ["PHOENIX_ENCRYPTION_KEYS_DIR"])
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 200, resp.text
body = resp.json()
# Paths land in the configured keys dir under the expected layout.
assert body["identity_path"].endswith("identity-rotation-2026-05-28.txt")
assert body["recipient_path"].endswith("rotation-2026-05-28.pub")
# Fingerprints are 16 lowercase hex chars and match each other
# (keygen primitive uses the recipient text for both sides).
assert len(body["identity_fingerprint"]) == 16
def test_admin_without_rotate_permission_gets_403(
self,
fake_pyrage: Any,
isolated_runtime: Path,
) -> None:
# Create an admin actor that is not allowed to rotate encryption keys.
actor = mint_bootstrap_actor("admin-no-rotate")
actor.is_admin = True
actor.can_rotate_encryption_key = False
payload = actor.to_payload()
auth_header = "Phoenix-Actor " + base64.b64encode(
json.dumps(payload).encode()
).decode("ascii")
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
headers={"Authorization": auth_header},
)
assert resp.status_code == 403, resp.text
body = resp.json()
# Admin status alone is not sufficient; missing rotate permission is enforced.
assert "can_rotate_encryption_key" in body.get("detail", "")If the test suite uses a shared helper to construct the Authorization header from an actor (e.g. a function wrapping the mint_bootstrap_actor(...).to_payload() + base64 logic), replace the inline header construction in test_admin_without_rotate_permission_gets_403 with that helper to match existing conventions.
If the error response format differs (for example, {"error": {"code": "...", "message": "..."}} or a different key than "detail"), update the final assertion to check the correct field and structure, keeping the core intent: asserting a 403 status and that the message indicates the missing can_rotate_encryption_key permission.
Comment on lines
+138
to
+147
| def test_happy_path_returns_200( | ||
| self, | ||
| fake_pyrage: Any, | ||
| isolated_runtime: Path, | ||
| ) -> None: | ||
| keys_dir = Path(os.environ["PHOENIX_ENCRYPTION_KEYS_DIR"]) | ||
| with TestClient(app) as client: | ||
| resp = client.post( | ||
| "/v1/admin/encryption/rotate-key", | ||
| json={"name": "rotation-2026-05-28"}, |
There was a problem hiding this comment.
suggestion (testing): Consider a test for keygen failure (KeyGenError/ImportError) returning 500 from the endpoint
To fully cover the endpoint’s documented behaviour, add an integration test where generate_age_keypair fails. For example, monkeypatch phoenix.admin.encryption_admin.generate_age_keypair to:
- Raise
KeyGenErrorand assert a 500 with the expected "keygen failed" detail - (Optionally) raise
ImportErrorand assert a 500 with the import error detail
This will verify the error-to-HTTP 500 mapping and make the failure modes reliable for clients.
Suggested implementation:
from phoenix.admin.encryption_admin import KeyGenError
class TestRotateKeyEndpoint: def test_happy_path_returns_200(
self,
fake_pyrage: Any,
isolated_runtime: Path,
) -> None:
keys_dir = Path(os.environ["PHOENIX_ENCRYPTION_KEYS_DIR"])
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 200
def test_keygen_error_returns_500(
self,
fake_pyrage: Any,
monkeypatch,
isolated_runtime: Path,
) -> None:
def _raise_keygen_error(*args, **kwargs):
raise KeyGenError("keygen failed")
import phoenix.admin.encryption_admin as encryption_admin
monkeypatch.setattr(
encryption_admin,
"generate_age_keypair",
_raise_keygen_error,
)
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 500
body = resp.json()
assert body["detail"] == "keygen failed"
def test_import_error_returns_500(
self,
fake_pyrage: Any,
monkeypatch,
isolated_runtime: Path,
) -> None:
def _raise_import_error(*args, **kwargs):
raise ImportError("age backend not available")
import phoenix.admin.encryption_admin as encryption_admin
monkeypatch.setattr(
encryption_admin,
"generate_age_keypair",
_raise_import_error,
)
with TestClient(app) as client:
resp = client.post(
"/v1/admin/encryption/rotate-key",
json={"name": "rotation-2026-05-28"},
)
assert resp.status_code == 500
body = resp.json()
assert "age backend not available" in body["detail"]- If
KeyGenErroris defined in a different module thanphoenix.admin.encryption_admin, adjust the import line accordingly (e.g.from phoenix.encryption import KeyGenError). - If the endpoint wraps error messages differently (for example, prefixes them or nests them under another field), tweak the assertions on
body["detail"]to match the actual response shape. - If the project uses a different fully-qualified path for
generate_age_keypair, update theimport phoenix.admin.encryption_admin as encryption_adminaccordingly so themonkeypatch.setattrtargets the correct function.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feba9d88c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+230
to
+233
| result = generate_age_keypair( | ||
| keys_dir=None, | ||
| name=name, | ||
| force=payload.force, |
There was a problem hiding this comment.
When an admin calls this rotate endpoint with {"name":"primary","force":true}, the shared keygen primitive resolves that name to identity.txt and recipients/primary.pub, so the endpoint overwrites the live identity rather than adding a rotation identity. After the advertised daemon restart, encryptor_from_default_layout() loads only identity.txt, and any existing ENCRYPTED_OPT_IN rows encrypted for the old primary key become undecryptable unless the operator has an out-of-band backup; the rotate endpoint should reject primary or otherwise avoid replacing the loaded identity.
Useful? React with 👍 / 👎.
Comment on lines
+192
to
+194
| identity_path.write_text(identity_text + "\n", encoding="utf-8") | ||
| if sys.platform != "win32": | ||
| os.chmod(identity_path, _POSIX_IDENTITY_MODE) |
There was a problem hiding this comment.
On POSIX systems with a permissive umask, write_text() creates the private age identity with the process default permissions before the later chmod(0o600). During that window, or if the process crashes between the write and chmod, the secret key can be left world/group-readable even though this helper is responsible for key material; open the file with mode 0o600 at creation time (and handle force atomically) so the secret is never written with broader permissions.
Useful? React with 👍 / 👎.
nah414
added a commit
that referenced
this pull request
May 29, 2026
Each Phoenix actor gets their own age identity + recipient pair. Ledger gains prompt_encryption_actor_id column (NULL = shared key, preserves 13.x.6 back-compat). Per-actor encryptor registry with disk-load fallback. CLI + admin endpoint gain --actor / actor_id. Four open tensions surfaced for Adam review: - actor-bootstrap (auto-generate on enroll vs opt-in admin command) - self-serve-rotation (admin-gated recommended) - cache-invalidation (evict on rotate recommended) - migration-direction (mixed-state ledger acceptable) Out of scope: batch migration of existing shared-key rows; cross-actor delegated decrypt; hardware-backed actor keys. Design-only this session; implementation deferred to future session after PR #23 (13.x.7) merges. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the two ergonomic gaps left by Phase 13.x.6 (PR #21) by shipping the
phoenix admin generate-encryption-keyCLI +POST /v1/admin/encryption/rotate-keyadmin endpoint, both backed by a sharedphoenix/ledger/keygen.py::generate_age_keypair()primitive.What ships
phoenix/ledger/keygen.py—GeneratedKeyPairdataclass +generate_age_keypair()+ 3 typed errors (KeyGenError/KeyGenPathConflict/KeyGenWriteError).phoenix admin generate-encryption-key [--name SLUG] [--force] [--keys-dir PATH].POST /v1/admin/encryption/rotate-keywith optional{name, force}body; returns paths + fingerprints +next_step.ActorPermissions.can_rotate_encryption_key(default deny; granted to bootstrap actors).admin.encryption.rotate.success+ 9 granularadmin.encryption.rotate.error.<kind>types.phoenix/ledger/encryption_age.pydrops the stale "(Phase 13.x.7 CLI)" parenthetical.phoenix/ledger/README.mdceremony docs reference the new CLI + endpoint (Setup section + Rotation section + What's-shipped/not-shipped table).NOT shipped (deferred follow-ups)
ENCRYPTED_OPT_INledger rows on rotation — substantial DB-transaction work; gets its own follow-up slot.POST /v1/admin/encryption/reloadzero-downtime encryptor reload — daemon-restart pattern preserved.Tests added
20 new across 4 test files:
tests/cognition/test_keygen.py(9) — primitive happy-path / conflict / force / fingerprint / POSIX-mode / name-validationtests/integration/test_admin_encryption_rotate_key.py(5) — endpoint happy-path / default-name / 409 / force / 403tests/cli/test_admin_generate_encryption_key.py(3) — CLI happy-path / conflict / forcetests/unit/test_permissions_phase13x7.py(3) — default-deny / explicit-grant / existing-flags-unchangedProject-wide pytest: 1316 passed, 43 skipped, 0 failures.
Spec / plan
docs/superpowers/specs/2026-05-28-phase-13x7-encryption-admin-design.md(99cf4f4on main)docs/superpowers/plans/2026-05-28-phase-13x7-encryption-admin.md(d8211b6on main)Test plan
pytest tests/cognition/test_keygen.py tests/integration/test_admin_encryption_rotate_key.py tests/cli/test_admin_generate_encryption_key.py tests/unit/test_permissions_phase13x7.py -vall greenmypy --strictclean on the 6 touched modulesnext_stepdaemon-restart message[1.1.0.dev0]below 13.x.4Generated with Claude Code
Summary by Sourcery
Add an encryption key generation primitive and expose it via both CLI and admin API, guarded by new permissions and documented in the ledger README and changelog.
New Features:
phoenix admin generate-encryption-keyCLI subcommand to generate local encryption keypairs without contacting the daemon.Enhancements:
Tests: