Update engine specs, audit doc, and server tests for SSH CA, transit, and user engines
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
121
AUDIT.md
121
AUDIT.md
@@ -105,12 +105,9 @@ User certificates should ideally include `source-address` critical options to li
|
||||
|
||||
Added `min_decryption_version` per key (default 1). Decryption requests for versions below the minimum are rejected. New `update-key-config` operation (admin-only) advances the minimum (can only increase, cannot exceed current version). New `trim-key` operation permanently deletes versions older than the minimum. Both have corresponding gRPC RPCs and REST endpoints. The rotation cycle is documented: rotate → rewrap → advance min → trim.
|
||||
|
||||
**16. Key version pruning with `max_key_versions` has no safety check**
|
||||
**16. ~~Key version pruning with `max_key_versions` has no safety check~~ RESOLVED**
|
||||
|
||||
If `max_key_versions` is set and data encrypted with an old version hasn't been re-wrapped, pruning that version makes the data permanently unrecoverable. There should be either:
|
||||
- A warning/confirmation mechanism, or
|
||||
- A way to scan for ciphertext referencing a version before pruning, or
|
||||
- At minimum, clear documentation that pruning is destructive.
|
||||
Added explicit `max_key_versions` behavior: auto-pruning during `rotate-key` only deletes versions strictly less than `min_decryption_version`. If the version count exceeds the limit but no eligible candidates remain, a warning is returned. This ensures pruning never destroys versions that may still have unrewrapped ciphertext. See also #30.
|
||||
|
||||
**17. ~~RSA encryption without specifying padding scheme~~ RESOLVED**
|
||||
|
||||
@@ -144,6 +141,106 @@ A compromised or malicious user token could issue unlimited encrypt/decrypt/sign
|
||||
|
||||
---
|
||||
|
||||
## Engine Design Review (2026-03-16)
|
||||
|
||||
**Scope**: engines/sshca.md, engines/transit.md, engines/user.md (patched specs)
|
||||
|
||||
### engines/sshca.md
|
||||
|
||||
#### Strengths
|
||||
|
||||
- RSA excluded — reduces attack surface, correct for SSH CA use case.
|
||||
- Detailed Go code snippets for Initialize, sign-host, sign-user flows.
|
||||
- KRL custom implementation correctly identified that `x/crypto/ssh` lacks KRL builders.
|
||||
- Signing profiles are the only path to critical options — good privilege separation.
|
||||
- Server-side serial generation with `crypto/rand` — no user-controllable serials.
|
||||
|
||||
#### Issues
|
||||
|
||||
**25. ~~Missing `list-certs` REST route~~ RESOLVED**
|
||||
|
||||
Added `GET /v1/sshca/{mount}/certs` to the REST endpoints table and route registration code block. API sync restored.
|
||||
|
||||
**26. ~~KRL section type description contradicts pseudocode~~ RESOLVED**
|
||||
|
||||
Fixed the description block to use `KRL_SECTION_CERTIFICATES (0x01)` for the outer section type, matching the pseudocode and the OpenSSH `PROTOCOL.krl` spec.
|
||||
|
||||
**27. ~~Policy check after certificate construction in sign-host~~ RESOLVED**
|
||||
|
||||
Reordered both `sign-host` and `sign-user` flows to perform the policy check before generating the serial and building the certificate. Serial generation now only happens after authorization succeeds.
|
||||
|
||||
### engines/transit.md
|
||||
|
||||
#### Strengths
|
||||
|
||||
- XChaCha20-Poly1305 (not ChaCha20-Poly1305) — correct for random nonce safety.
|
||||
- All nonce sizes, hash algorithms, and signature encodings now specified.
|
||||
- `trim-key` logic is detailed and safe (no-op when `min_decryption_version` is 1).
|
||||
- Batch operations hold a read lock for atomicity with respect to key rotation.
|
||||
- 500-item batch limit prevents resource exhaustion.
|
||||
|
||||
#### Issues
|
||||
|
||||
**28. ~~HMAC output not versioned — unverifiable after key rotation~~ RESOLVED**
|
||||
|
||||
HMAC output now uses the same `metacrypt:v{version}:{base64}` format as ciphertext and signatures. Verification parses the version prefix, loads the corresponding key (subject to `min_decryption_version`), and uses `hmac.Equal` for constant-time comparison.
|
||||
|
||||
**29. ~~`rewrap` policy action not specified~~ RESOLVED**
|
||||
|
||||
`rewrap` and `batch-rewrap` now map to the `decrypt` action — rewrap internally decrypts and re-encrypts, so the caller must have decrypt permission. Batch variants map to the same action as their single counterparts. Documented in the authorization section.
|
||||
|
||||
**30. ~~`max_key_versions` interaction with `min_decryption_version` unclear~~ RESOLVED**
|
||||
|
||||
Added explicit `max_key_versions` behavior section. Pruning happens during `rotate-key` and only deletes versions strictly less than `min_decryption_version`. If the limit is exceeded but no eligible candidates remain, a warning is returned. This also resolves audit finding #16.
|
||||
|
||||
**31. ~~Missing `get-public-key` REST route~~ RESOLVED**
|
||||
|
||||
Added `GET /v1/transit/{mount}/keys/{name}/public-key` to the REST endpoints table and route registration code block. API sync restored.
|
||||
|
||||
**32. ~~`exportable` flag with no export operation~~ RESOLVED**
|
||||
|
||||
Removed the `exportable` flag from `create-key`. Transit's value proposition is that keys never leave the service. If export is needed for migration, a dedicated admin-only operation can be added later with audit logging.
|
||||
|
||||
### engines/user.md
|
||||
|
||||
#### Strengths
|
||||
|
||||
- HKDF with per-recipient random salt — prevents wrapping key reuse across messages.
|
||||
- AES-256-GCM for DEK wrapping (consistent with codebase, avoids new primitive).
|
||||
- ECDH key agreement with info-string binding prevents key confusion.
|
||||
- Explicit zeroization of all intermediate secrets documented.
|
||||
- Envelope format includes salt per-recipient — correct for HKDF security.
|
||||
|
||||
#### Issues
|
||||
|
||||
**33. ~~Auto-provisioning creates keys for arbitrary usernames~~ RESOLVED**
|
||||
|
||||
The encrypt flow now validates recipient usernames against MCIAS via `auth.ValidateUsername` before auto-provisioning. Non-existent usernames are rejected with an error, preventing barrier pollution.
|
||||
|
||||
**34. ~~No recipient limit on encrypt~~ RESOLVED**
|
||||
|
||||
Added a `maxRecipients = 100` limit. Requests exceeding this limit are rejected with `400 Bad Request` before any ECDH computation.
|
||||
|
||||
**35. ~~No re-encryption support for key rotation~~ RESOLVED**
|
||||
|
||||
Added a `re-encrypt` operation that decrypts an envelope and re-encrypts it with current key pairs for all recipients. This enables safe key rotation: re-encrypt all stored envelopes first, then call `rotate-key`. Added to HandleRequest dispatch, gRPC service, REST endpoints, and route registration.
|
||||
|
||||
**36. ~~`UserKeyConfig` type undefined~~ RESOLVED**
|
||||
|
||||
Defined `UserKeyConfig` struct with `Algorithm`, `CreatedAt`, and `AutoProvisioned` fields in the in-memory state section.
|
||||
|
||||
### Cross-Cutting Issues (Engine Designs)
|
||||
|
||||
**37. ~~`adminOnlyOperations` name collision blocks user engine `rotate-key`~~ RESOLVED**
|
||||
|
||||
Changed the `adminOnlyOperations` map from flat operation names to engine-type-qualified keys (`engineType:operation`, e.g. `"transit:rotate-key"`). The generic endpoint now resolves the mount's engine type via `GetMount` before checking the map. Added tests verifying that `rotate-key` on a user mount succeeds for non-admin users while `rotate-key` on a transit mount correctly requires admin.
|
||||
|
||||
**38. ~~`engine.ZeroizeKey` helper prerequisite not cross-referenced~~ RESOLVED**
|
||||
|
||||
Added prerequisite step to both transit and user implementation steps referencing `engines/sshca.md` step 1 for the `engine.ZeroizeKey` shared helper.
|
||||
|
||||
---
|
||||
|
||||
## Priority Summary
|
||||
|
||||
| Priority | Issue | Location |
|
||||
@@ -151,6 +248,7 @@ A compromised or malicious user token could issue unlimited encrypt/decrypt/sign
|
||||
| ~~**Critical**~~ | ~~#4 — Policy auth contradiction (admin vs user)~~ **RESOLVED** | ARCHITECTURE.md |
|
||||
| ~~**Critical**~~ | ~~#9 — User-controllable SSH cert serials~~ **RESOLVED** | sshca.md |
|
||||
| ~~**Critical**~~ | ~~#13 — Policy path collision (`ca/` vs `sshca/`)~~ **RESOLVED** | sshca.md |
|
||||
| ~~**Critical**~~ | ~~#37 — `adminOnlyOperations` name collision blocks user `rotate-key`~~ **RESOLVED** | Cross-cutting |
|
||||
| ~~**High**~~ | ~~#5 — No path AAD in barrier encryption~~ **RESOLVED** | ARCHITECTURE.md |
|
||||
| ~~**High**~~ | ~~#12 — No KRL distribution for SSH revocation~~ **RESOLVED** | sshca.md |
|
||||
| ~~**High**~~ | ~~#15 — No min key version for transit rotation~~ **RESOLVED** | transit.md |
|
||||
@@ -158,12 +256,25 @@ A compromised or malicious user token could issue unlimited encrypt/decrypt/sign
|
||||
| ~~**High**~~ | ~~#11 — `critical_options` not restricted~~ **RESOLVED** | sshca.md |
|
||||
| ~~**High**~~ | ~~#6 — Single MEK with no rotation~~ **RESOLVED** | ARCHITECTURE.md |
|
||||
| ~~**High**~~ | ~~#22 — No forward secrecy / per-engine DEKs~~ **RESOLVED** | Cross-cutting |
|
||||
| ~~**High**~~ | ~~#28 — HMAC output not versioned~~ **RESOLVED** | transit.md |
|
||||
| ~~**High**~~ | ~~#30 — `max_key_versions` vs `min_decryption_version` unclear~~ **RESOLVED** | transit.md |
|
||||
| ~~**High**~~ | ~~#33 — Auto-provision creates keys for arbitrary usernames~~ **RESOLVED** | user.md |
|
||||
| ~~**Medium**~~ | ~~#2 — Token cache revocation gap~~ **ACCEPTED** | ARCHITECTURE.md |
|
||||
| ~~**Medium**~~ | ~~#3 — Admin all-or-nothing access~~ **ACCEPTED** | ARCHITECTURE.md |
|
||||
| ~~**Medium**~~ | ~~#8 — Unseal rate limit resets on restart~~ **ACCEPTED** | ARCHITECTURE.md |
|
||||
| ~~**Medium**~~ | ~~#20 — `decrypt` mapped to `read` action~~ **RESOLVED** | transit.md |
|
||||
| ~~**Medium**~~ | ~~#24 — No CSRF protection for web UI~~ **RESOLVED** | ARCHITECTURE.md |
|
||||
| ~~**Medium**~~ | ~~#25 — Missing `list-certs` REST route~~ **RESOLVED** | sshca.md |
|
||||
| ~~**Medium**~~ | ~~#26 — KRL section type description error~~ **RESOLVED** | sshca.md |
|
||||
| ~~**Medium**~~ | ~~#27 — Policy check after cert construction~~ **RESOLVED** | sshca.md |
|
||||
| ~~**Medium**~~ | ~~#29 — `rewrap` policy action not specified~~ **RESOLVED** | transit.md |
|
||||
| ~~**Medium**~~ | ~~#31 — Missing `get-public-key` REST route~~ **RESOLVED** | transit.md |
|
||||
| ~~**Medium**~~ | ~~#34 — No recipient limit on encrypt~~ **RESOLVED** | user.md |
|
||||
| ~~**Low**~~ | ~~#1 — TLS 1.2 vs 1.3~~ **RESOLVED** | ARCHITECTURE.md |
|
||||
| ~~**Low**~~ | ~~#19 — No batch transit operations~~ **RESOLVED** | transit.md |
|
||||
| ~~**Low**~~ | ~~#18 — HMAC/sign semantic confusion~~ **RESOLVED** | transit.md |
|
||||
| ~~**Medium**~~ | ~~#23 — Generic endpoint bypasses typed route middleware~~ **RESOLVED** | Cross-cutting |
|
||||
| ~~**Low**~~ | ~~#32 — `exportable` flag with no export operation~~ **RESOLVED** | transit.md |
|
||||
| ~~**Low**~~ | ~~#35 — No re-encryption support for user key rotation~~ **RESOLVED** | user.md |
|
||||
| ~~**Low**~~ | ~~#36 — `UserKeyConfig` type undefined~~ **RESOLVED** | user.md |
|
||||
| ~~**Low**~~ | ~~#38 — `ZeroizeKey` prerequisite not cross-referenced~~ **RESOLVED** | Cross-cutting |
|
||||
|
||||
Reference in New Issue
Block a user