tasks: decompose vault, core, call crates into 28 atomic implementation tasks
Break down the three initial crates (alknet-vault, alknet-core, alknet-call) into dependency-ordered task files for implementation agents. Structure: - tasks/vault/ (10 tasks) — drift fixes from ADR-025/026 refactor, review, spec sync. Vault is independent and can run fully in parallel with core/call. - tasks/core/ (6 tasks) — crate init, core types, config, auth, endpoint, review. Core is foundational; call depends on it. - tasks/call/ (12 tasks) — split into registry/ and protocol/ topic subdirs reflecting the two subsystems. CallAdapter is the merge point. Key decisions: - Drifts 3+9+10 grouped as one task (key-versioning-rotation) — the complete ADR-021 rotation feature that doesn't compile in pieces - Reviews injected at end of each crate phase (vault, core, call) - Vault spec-sync task removes the drift table and bumps doc status to stable - ACME deferred in core/endpoint (noted as TODO; X509 manual certs for now) - OperationEnv kept as a trait (load-bearing for ADR-024 layering) Validated: 28 tasks, no cycles, 11 generations of parallel work. Critical path runs through call (11 tasks). Vault completes by generation 4. 6 high-risk tasks identified (21%): irpc-removal, endpoint, operation-context, operation-env, call-adapter, abort-cascade.
This commit is contained in:
85
tasks/vault/cache-zeroization-test.md
Normal file
85
tasks/vault/cache-zeroization-test.md
Normal file
@@ -0,0 +1,85 @@
|
||||
---
|
||||
id: vault/cache-zeroization-test
|
||||
name: Verify and test that HashMap::clear() drops CachedKey values triggering zeroization
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: single
|
||||
risk: low
|
||||
impact: isolated
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Fix drift item #6: `KeyCache::clear()` removes entries and relies on
|
||||
`CachedKey`'s `Drop` impl for zeroization. The spec says to verify that
|
||||
`HashMap::clear()` actually drops the values (it does, but this is worth a
|
||||
test). This task adds a test that proves zeroization happens on cache eviction
|
||||
and clear.
|
||||
|
||||
### Background
|
||||
|
||||
`CachedKey` derives `Zeroize` and `ZeroizeOnDrop` (via the `DerivedKey` it
|
||||
holds, which is `#[zeroize(drop)]`). When the cache evicts an entry (LRU or TTL)
|
||||
or `clear()` is called, the `CachedKey` is dropped, which triggers
|
||||
`ZeroizeOnDrop` — the private key bytes are zeroized before deallocation.
|
||||
|
||||
`HashMap::clear()` drops all values, which triggers their `Drop` impls. This
|
||||
is standard Rust behavior, but the security-critical nature of key material
|
||||
warrants an explicit test.
|
||||
|
||||
### What to add
|
||||
|
||||
A test in `cache.rs` (or `tests/`) that:
|
||||
|
||||
1. Inserts a `CachedKey` with a known private key into the cache
|
||||
2. Verifies the key is present
|
||||
3. Calls `clear()` (or evicts via LRU/TTL)
|
||||
4. Verifies the `CachedKey` was dropped and zeroized
|
||||
|
||||
Testing zeroization directly is tricky because the memory is freed — you can't
|
||||
easily inspect it after drop. A practical approach:
|
||||
|
||||
- **Option A**: Use a custom type with a `Drop` impl that sets a flag (e.g., an
|
||||
`Arc<AtomicBool>`) when zeroized. Insert it into the cache, clear, verify the
|
||||
flag is set. This tests the drop path, not the zeroize path directly, but
|
||||
confirms `clear()` drops values.
|
||||
- **Option B**: Test the LRU eviction path — fill the cache to `max_entries`,
|
||||
insert one more, verify the LRU entry was evicted (dropped).
|
||||
- **Option C**: Test that `lock()` calls `cache.clear()` and the cache is empty
|
||||
afterward (integration test via `VaultServiceHandle`).
|
||||
|
||||
At minimum, implement Option B and C. Option A is a bonus if feasible without
|
||||
over-engineering the test type.
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches `cache.rs` (test additions) and possibly `tests/`. It does
|
||||
not depend on the irpc removal task (drift #4) because `cache.rs` is a separate
|
||||
file. It can run in parallel with drift #4.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Test: LRU eviction drops the evicted `CachedKey` (cache exceeds `max_entries`, oldest evicted)
|
||||
- [ ] Test: `lock()` clears the cache (verify cache is empty after lock)
|
||||
- [ ] Test: TTL expiry evicts entries (set short TTL, wait, verify entry gone)
|
||||
- [ ] Test: `clear()` removes all entries (verify empty after clear)
|
||||
- [ ] `cargo test` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table item #6
|
||||
- docs/architecture/crates/vault/service.md — Cache section, Security Constraints
|
||||
- docs/architecture/crates/vault/encryption.md — Security Constraints
|
||||
|
||||
## Notes
|
||||
|
||||
> `HashMap::clear()` does drop values, triggering their `Drop` impls. This is
|
||||
> standard Rust behavior, but key material is security-critical enough to
|
||||
> warrant an explicit test. This task touches only `cache.rs` and can run in
|
||||
> parallel with the irpc removal task (drift #4).
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
140
tasks/vault/derivedkey-serialization.md
Normal file
140
tasks/vault/derivedkey-serialization.md
Normal file
@@ -0,0 +1,140 @@
|
||||
---
|
||||
id: vault/derivedkey-serialization
|
||||
name: Implement always-redact DerivedKey serialization and reject redacted payloads on deserialize
|
||||
status: pending
|
||||
depends_on: [vault/irpc-removal]
|
||||
scope: narrow
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Fix drift item #5: `DerivedKey` currently has dual serialization behavior — JSON
|
||||
redacts the private key, but postcard (the binary format used by irpc) preserves
|
||||
the raw bytes. ADR-025 dropped the postcard/remote path, so `DerivedKey` should
|
||||
**always** redact on serialize and reject `"[REDACTED]"` on deserialize with an
|
||||
explicit error.
|
||||
|
||||
### Current state
|
||||
|
||||
`protocol.rs` has `DerivedKey` with `#[derive(Serialize, Deserialize)]` (or
|
||||
similar) that produces JSON redaction for JSON but preserves bytes in postcard.
|
||||
The postcard tests in the test suite verify the binary round-trip.
|
||||
|
||||
### Target state
|
||||
|
||||
Per `docs/architecture/crates/vault/protocol.md` → Serialization Redaction:
|
||||
|
||||
`DerivedKey` must **not** derive `Deserialize` via `#[derive]`. It needs custom
|
||||
`Serialize` and `Deserialize` impls:
|
||||
|
||||
**Custom Serialize** — always redacts `private_key`:
|
||||
```rust
|
||||
impl serde::Serialize for DerivedKey {
|
||||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
|
||||
where S: serde::Serializer {
|
||||
use serde::SerializeStruct;
|
||||
let mut s = serializer.serialize_struct("DerivedKey", 3)?;
|
||||
s.serialize_field("key_type", &self.key_type)?;
|
||||
s.serialize_field("private_key", "[REDACTED]")?;
|
||||
s.serialize_field("public_key", &self.public_key)?;
|
||||
s.end()
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Custom Deserialize** — rejects `"[REDACTED]"` with an explicit error:
|
||||
```rust
|
||||
impl<'de> serde::Deserialize<'de> for DerivedKey {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
where D: serde::Deserializer<'de> {
|
||||
#[derive(serde::Deserialize)]
|
||||
struct DerivedKeyHelper {
|
||||
key_type: KeyType,
|
||||
private_key: Vec<u8>,
|
||||
public_key: Vec<u8>,
|
||||
}
|
||||
let helper = DerivedKeyHelper::deserialize(deserializer)?;
|
||||
if helper.private_key == b"[REDACTED]" {
|
||||
return Err(serde::de::Error::custom(
|
||||
"DerivedKey.private_key is \"[REDACTED]\" — redacted payloads \
|
||||
cannot be deserialized. JSON round-tripping a DerivedKey is \
|
||||
not supported (the private key is gone)."
|
||||
));
|
||||
}
|
||||
Ok(DerivedKey {
|
||||
key_type: helper.key_type,
|
||||
private_key: helper.private_key,
|
||||
public_key: helper.public_key,
|
||||
})
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Debug impl** — also redacts:
|
||||
```rust
|
||||
impl fmt::Debug for DerivedKey {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
f.debug_struct("DerivedKey")
|
||||
.field("key_type", &self.key_type)
|
||||
.field("private_key", &"[REDACTED]")
|
||||
.field("public_key", &self.public_key)
|
||||
.finish()
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Remove postcard tests
|
||||
|
||||
The postcard round-trip tests (which verified binary format preserved private
|
||||
key bytes) are removed — ADR-025 dropped that path. The `postcard`
|
||||
dev-dependency was removed in the irpc removal task (drift #4).
|
||||
|
||||
### Why custom impls instead of derives
|
||||
|
||||
A derived `Deserialize` would generate a default impl that conflicts with the
|
||||
manual one, and would only fail incidentally (serde type mismatch: string vs
|
||||
sequence), not with the explicit redaction-rejection error the spec requires.
|
||||
The custom impl is required for the explicit error message.
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches `protocol.rs` (the `DerivedKey` type, its serde impls, Debug
|
||||
impl) and test files (remove postcard tests, add redaction tests). It depends on
|
||||
the irpc removal task (drift #4) because both modify `protocol.rs`.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `DerivedKey` does not derive `Serialize` or `Deserialize` via `#[derive]`
|
||||
- [ ] Custom `Serialize` impl always redacts `private_key` as `"[REDACTED]"`
|
||||
- [ ] Custom `Deserialize` impl rejects `private_key == b"[REDACTED]"` with explicit error
|
||||
- [ ] Custom `Debug` impl redacts `private_key` as `"[REDACTED]"`
|
||||
- [ ] Postcard round-trip tests removed
|
||||
- [ ] Unit test: JSON serialize produces `"[REDACTED]"` for `private_key`
|
||||
- [ ] Unit test: JSON deserialize of a redacted payload returns an error (not a corrupted key)
|
||||
- [ ] Unit test: `{:?}` on `DerivedKey` does not contain private key bytes
|
||||
- [ ] `cargo test` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table item #5
|
||||
- docs/architecture/crates/vault/protocol.md — Serialization Redaction, Debug redaction
|
||||
- docs/architecture/decisions/025-vault-local-only-dispatch.md — ADR-025 (resolves W8)
|
||||
- docs/architecture/decisions/014-secret-material-flow-and-capability-injection.md — ADR-014
|
||||
|
||||
## Notes
|
||||
|
||||
> The redaction is defense-in-depth for logging safety, not the primary control
|
||||
> — the primary control is that `DerivedKey` never crosses the call protocol
|
||||
> wire (ADR-014). ADR-025 dropped the postcard/remote path that previously
|
||||
> preserved bytes in binary formats. The custom Deserialize impl is required
|
||||
> because `#[derive(Deserialize)]` would conflict and not produce the explicit
|
||||
> redaction-rejection error. Depends on irpc removal because both modify
|
||||
> `protocol.rs`.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
106
tasks/vault/irpc-removal.md
Normal file
106
tasks/vault/irpc-removal.md
Normal file
@@ -0,0 +1,106 @@
|
||||
---
|
||||
id: vault/irpc-removal
|
||||
name: Remove irpc dependency and actor dispatch from vault, convert to direct method calls on VaultServiceHandle
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: broad
|
||||
risk: high
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Remove the irpc-based actor dispatch from the vault crate and convert to direct
|
||||
method calls on `VaultServiceHandle`. This is drift item #4 from the vault README
|
||||
drift table and the foundational ADR-025 refactor — it restructures `service.rs`
|
||||
and `protocol.rs` fundamentally, which is why most other vault drift tasks depend
|
||||
on this one.
|
||||
|
||||
### What to remove
|
||||
|
||||
- `VaultProtocol` enum with `#[rpc_requests]` derive in `protocol.rs`
|
||||
- `VaultServiceActor` in `service.rs`
|
||||
- `Client<VaultProtocol>` usage
|
||||
- `irpc` and `irpc-derive` dependencies from `Cargo.toml`
|
||||
- `postcard` from dev-dependencies (was only needed for the irpc binary path)
|
||||
- `tokio` dependency from `Cargo.toml` (the vault uses `std::sync::RwLock`, not
|
||||
`tokio::sync::RwLock` — ADR-025)
|
||||
- `VaultMessage` / `VaultProtocol` re-exports from `lib.rs`
|
||||
|
||||
### What to keep / change
|
||||
|
||||
- `VaultServiceHandle` stays — it becomes the sole API. It is already
|
||||
`Arc<std::sync::RwLock<VaultServiceInner>>` with synchronous methods. The actor
|
||||
path (`mpsc` channel + oneshot backchannels via irpc's `Service` trait) is
|
||||
removed entirely.
|
||||
- `VaultServiceError` drops `Serialize`/`Deserialize` derives (were needed for
|
||||
irpc dispatch — ADR-025 removed that path). It becomes a plain `thiserror::Error`
|
||||
enum.
|
||||
- `DerivedKey` and `KeyType` stay in `protocol.rs` — the file is renamed in
|
||||
spirit to "the types module" but the filename stays `protocol.rs` for
|
||||
continuity. The `VaultProtocol` enum is removed; `DerivedKey`/`KeyType` remain.
|
||||
- `lib.rs` re-exports are updated to remove `VaultMessage`, `VaultProtocol`,
|
||||
`VaultServiceActor` and reflect the new public API per the vault README's
|
||||
Public API section.
|
||||
|
||||
### Public API after this task
|
||||
|
||||
Per `docs/architecture/crates/vault/README.md` → Public API:
|
||||
|
||||
```rust
|
||||
pub use mnemonic::{Language, Mnemonic, Seed};
|
||||
pub use derivation::{DerivationError, ExtendedPrivKey, PATHS};
|
||||
pub use encryption::{EncryptedData, EncryptionError, EncryptionKey};
|
||||
pub use encryption::CURRENT_KEY_VERSION;
|
||||
pub use protocol::{DerivedKey, KeyType};
|
||||
pub use service::{VaultServiceError, VaultServiceHandle};
|
||||
pub use cache::CacheConfig;
|
||||
```
|
||||
|
||||
### Cargo.toml changes
|
||||
|
||||
Remove from `[dependencies]`:
|
||||
- `irpc = { workspace = true }`
|
||||
- `irpc-derive = { workspace = true }`
|
||||
- `tokio = { version = "1", features = ["sync", "rt", "macros"] }`
|
||||
|
||||
Remove from `[dev-dependencies]`:
|
||||
- `postcard = { version = "1", features = ["alloc"] }`
|
||||
|
||||
The vault should have **zero** async runtime dependency after this task.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `VaultProtocol` enum and `#[rpc_requests]` derive removed from `protocol.rs`
|
||||
- [ ] `VaultServiceActor` removed from `service.rs`
|
||||
- [ ] `Client<VaultProtocol>` usage removed
|
||||
- [ ] `irpc`, `irpc-derive`, `tokio` removed from `[dependencies]` in `Cargo.toml`
|
||||
- [ ] `postcard` removed from `[dev-dependencies]` in `Cargo.toml`
|
||||
- [ ] `VaultServiceError` no longer derives `Serialize`/`Deserialize`
|
||||
- [ ] `lib.rs` re-exports match the Public API section of vault README (no `VaultMessage`, `VaultProtocol`, `VaultServiceActor`)
|
||||
- [ ] `VaultServiceHandle` methods are all synchronous (no `async`, no `.await`)
|
||||
- [ ] `cargo check` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
- [ ] `cargo test` succeeds (existing tests updated to remove irpc/postcard usage)
|
||||
- [ ] No `tokio` dependency remains in the vault `Cargo.toml`
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table item #4, Public API section
|
||||
- docs/architecture/crates/vault/service.md — Dispatch section, VaultServiceHandle
|
||||
- docs/architecture/crates/vault/protocol.md — Local-Only by Construction
|
||||
- docs/architecture/decisions/025-vault-local-only-dispatch.md — ADR-025
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the foundational vault refactor. It restructures `service.rs` and
|
||||
> `protocol.rs` — most other vault drift tasks touch these same files and must
|
||||
> follow this one to avoid merge conflicts. The `VaultServiceHandle` struct
|
||||
> already uses `std::sync::RwLock` with synchronous methods; the actor path is
|
||||
> the dead code to remove. After this task, the vault has no async runtime
|
||||
> dependency and no RPC framework dependency — it is local-only by construction.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
127
tasks/vault/key-versioning-rotation.md
Normal file
127
tasks/vault/key-versioning-rotation.md
Normal file
@@ -0,0 +1,127 @@
|
||||
---
|
||||
id: vault/key-versioning-rotation
|
||||
name: Implement version-indexed encryption key paths, bump CURRENT_KEY_VERSION to 2, and add rotate method
|
||||
status: pending
|
||||
depends_on: [vault/irpc-removal]
|
||||
scope: moderate
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Fix drift items #3, #9, and #10 as one coherent feature: the version-indexed
|
||||
key rotation mechanism from ADR-021. These three drifts are tightly coupled —
|
||||
`CURRENT_KEY_VERSION = 2` (drift #3), version-aware `encrypt`/`decrypt` via
|
||||
`encryption_path_for_version` (drift #9), and the `rotate` method (drift #10)
|
||||
form the complete key rotation feature. Splitting them would produce tasks that
|
||||
don't compile independently.
|
||||
|
||||
### Drift #3: Bump CURRENT_KEY_VERSION
|
||||
|
||||
Current: `CURRENT_KEY_VERSION = 1` (but the key is HD-derived, and v1 is
|
||||
reserved for the TypeScript PBKDF2 legacy per ADR-020).
|
||||
|
||||
Target: `CURRENT_KEY_VERSION = 2` (HD-derived, per ADR-020).
|
||||
|
||||
Version semantics:
|
||||
- v1: TypeScript predecessor's PBKDF2-encrypted data — the vault **cannot**
|
||||
decrypt it (different key derivation). Migration is a one-time re-encryption.
|
||||
- v2: HD-derived at `m/74'/2'/0'/0'` (PATHS::ENCRYPTION) — current.
|
||||
- v3+: `m/74'/2'/0'/1'`, `m/74'/2'/0'/2'`, etc. — future rotation versions.
|
||||
|
||||
### Drift #9: Version-aware encrypt/decrypt
|
||||
|
||||
Current: `encrypt`/`decrypt` always derive at `PATHS::ENCRYPTION` regardless of
|
||||
the `key_version` parameter.
|
||||
|
||||
Target:
|
||||
- `encrypt(plaintext, key_version)`: derive the encryption key at
|
||||
`encryption_path_for_version(key_version)`, stamp the same `key_version` on
|
||||
the resulting `EncryptedData`.
|
||||
- `decrypt(encrypted)`: derive the key at
|
||||
`encryption_path_for_version(encrypted.key_version)` — the blob carries its
|
||||
own version, and each version maps to a distinct derivation path.
|
||||
|
||||
This requires:
|
||||
1. `encryption_path_for_version(version: u32) -> Result<String, DerivationError>`
|
||||
already exists in `derivation.rs` — verify it returns `InvalidPath` for
|
||||
`version < 2` (v1 is TS legacy, v0 is meaningless).
|
||||
2. `derive_encryption_key_for_version(version: u32) -> Result<DerivedKey, VaultServiceError>`
|
||||
— a new method on `VaultServiceHandle` that maps version → path → derive.
|
||||
Cached by path (same cache as `derive_encryption_key`).
|
||||
3. `encrypt` and `decrypt` use `derive_encryption_key_for_version` instead of
|
||||
deriving at the fixed `PATHS::ENCRYPTION` path.
|
||||
|
||||
### Drift #10: Implement rotate
|
||||
|
||||
Current: no `rotate` method exists.
|
||||
|
||||
Target:
|
||||
```rust
|
||||
pub fn rotate(&self, encrypted: &EncryptedData, to_version: u32) -> Result<EncryptedData, VaultServiceError>;
|
||||
```
|
||||
|
||||
Decrypts with the old version's key (from `encrypted.key_version`), re-encrypts
|
||||
with the new version's key (`to_version`). Returns the new `EncryptedData` —
|
||||
the caller replaces the blob in storage. No new mnemonic needed; the same seed
|
||||
produces all version keys via different derivation paths (ADR-021).
|
||||
|
||||
### Implementation notes
|
||||
|
||||
- `derive_encryption_key(path)` (the path-based API) remains as-is for deriving
|
||||
at arbitrary paths. `derive_encryption_key_for_version(version)` is the
|
||||
version-aware API used by `encrypt`/`decrypt`. Both share the same cache
|
||||
(keyed by derivation path).
|
||||
- `encrypt` and `decrypt` extract the `EncryptionKey` from the `DerivedKey` via
|
||||
`EncryptionKey::from_derived_bytes` (see encryption.md).
|
||||
- `encryption_path_for_version` returns `InvalidPath` for `version < 2`.
|
||||
`derive_encryption_key_for_version` propagates this as
|
||||
`VaultServiceError::InvalidPath`.
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches `encryption.rs` (CURRENT_KEY_VERSION), `service.rs` (encrypt,
|
||||
decrypt, rotate, derive_encryption_key_for_version), and possibly `derivation.rs`
|
||||
(verify `encryption_path_for_version`). It depends on the irpc removal task
|
||||
(drift #4) because both modify `service.rs`.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `CURRENT_KEY_VERSION` is `2` in `encryption.rs`
|
||||
- [ ] `derive_encryption_key_for_version(version)` method added to `VaultServiceHandle`
|
||||
- [ ] `derive_encryption_key_for_version` returns `InvalidPath` for `version < 2`
|
||||
- [ ] `encrypt(plaintext, key_version)` derives at `encryption_path_for_version(key_version)`
|
||||
- [ ] `encrypt` stamps the passed `key_version` on the resulting `EncryptedData`
|
||||
- [ ] `decrypt(encrypted)` derives at `encryption_path_for_version(encrypted.key_version)`
|
||||
- [ ] `rotate(encrypted, to_version)` method implemented: decrypt old, re-encrypt new
|
||||
- [ ] `rotate` returns `EncryptedData` with `key_version = to_version`
|
||||
- [ ] Unit test: encrypt at v2, decrypt at v2 — round-trip succeeds
|
||||
- [ ] Unit test: encrypt at v2, rotate to v3, decrypt at v3 — round-trip succeeds
|
||||
- [ ] Unit test: decrypt v2 blob after rotation — old key still derivable (partial rotation safe)
|
||||
- [ ] Unit test: `derive_encryption_key_for_version(1)` returns `InvalidPath`
|
||||
- [ ] Unit test: `derive_encryption_key_for_version(0)` returns `InvalidPath`
|
||||
- [ ] `cargo test` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table items #3, #9, #10
|
||||
- docs/architecture/crates/vault/encryption.md — Key Versioning, Rotation, EncryptionKey
|
||||
- docs/architecture/crates/vault/service.md — encrypt, decrypt, rotate, derive_encryption_key_for_version
|
||||
- docs/architecture/crates/vault/mnemonic-derivation.md — encryption_path_for_version, PATHS
|
||||
- docs/architecture/decisions/020-hd-derivation-for-encryption-keys.md — ADR-020
|
||||
- docs/architecture/decisions/021-key-rotation-via-version-indexed-paths.md — ADR-021
|
||||
|
||||
## Notes
|
||||
|
||||
> These three drifts are one feature: version-indexed key rotation (ADR-021).
|
||||
> Splitting them would produce tasks that don't compile independently —
|
||||
> bumping the version without version-aware encrypt/decrypt would make v2
|
||||
> blobs undecryptable, and rotate without version-aware encrypt/decrypt has no
|
||||
> keys to work with. Depends on irpc removal because both modify `service.rs`.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
83
tasks/vault/osrng-iv-generation.md
Normal file
83
tasks/vault/osrng-iv-generation.md
Normal file
@@ -0,0 +1,83 @@
|
||||
---
|
||||
id: vault/osrng-iv-generation
|
||||
name: Replace rand::random() IV generation with OsRng in AES-GCM encryption
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: single
|
||||
risk: medium
|
||||
impact: isolated
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Fix drift item #1: the AES-256-GCM IV (nonce) generation in `encryption.rs`
|
||||
currently uses `rand::random()`, which uses the thread-local RNG and may not be a
|
||||
CSPRNG on all platforms. Replace with `OsRng` (or equivalent CSPRNG).
|
||||
|
||||
This is a security-critical fix. IV reuse under the same AES-GCM key is
|
||||
catastrophic — it breaks authenticity and creates a two-time-pad on the
|
||||
plaintext. `OsRng` reads from the operating system's entropy source and is the
|
||||
correct choice for cryptographic nonces.
|
||||
|
||||
### Current state
|
||||
|
||||
`encryption.rs` line ~133: IV generation uses `rand::random()` to produce the
|
||||
12-byte GCM nonce.
|
||||
|
||||
### Target state
|
||||
|
||||
Use `rand::rngs::OsRng` (from the `rand` crate, which is already a dependency)
|
||||
to generate the 12-byte IV. The `aes-gcm` crate's `Aes256Gcm` encrypt path takes
|
||||
a `Nonce` — construct it from `OsRng`-generated bytes.
|
||||
|
||||
```rust
|
||||
use rand::rngs::OsRng;
|
||||
use rand::RngCore;
|
||||
|
||||
let mut iv_bytes = [0u8; 12];
|
||||
OsRng.fill_bytes(&mut iv_bytes);
|
||||
let nonce = Nonce::from_slice(&iv_bytes);
|
||||
```
|
||||
|
||||
The IV is generated fresh for each `encrypt()` call. The salt (32 bytes, unused
|
||||
in v2 for key derivation but kept for wire-format compat) should also use `OsRng`
|
||||
for consistency — it's stored in the `EncryptedData` blob and doesn't need to be
|
||||
deterministic.
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches only `encryption.rs`. It does not depend on the irpc removal
|
||||
(drift #4) because `encryption.rs` is a separate file from `service.rs` /
|
||||
`protocol.rs`. It can run in parallel with drift #4.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `encryption::encrypt()` uses `OsRng` for IV generation, not `rand::random()`
|
||||
- [ ] Salt generation uses `OsRng` (or equivalent CSPRNG)
|
||||
- [ ] No `rand::random()` calls remain in `encryption.rs`
|
||||
- [ ] IV is 12 bytes (standard GCM nonce size)
|
||||
- [ ] Salt is 32 bytes (wire-format compat, unused in key derivation)
|
||||
- [ ] Unit test: verify IV is fresh on each encrypt call (encrypt twice, different IVs)
|
||||
- [ ] Unit test: verify decrypt round-trip still works after the change
|
||||
- [ ] `cargo test` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table item #1
|
||||
- docs/architecture/crates/vault/encryption.md — Security Constraints: OsRng for IVs
|
||||
- docs/architecture/crates/vault/service.md — Security Constraints: OsRng for IVs
|
||||
- docs/architecture/decisions/020-hd-derivation-for-encryption-keys.md — ADR-020
|
||||
|
||||
## Notes
|
||||
|
||||
> This is a security-critical fix. IV reuse under the same AES-GCM key breaks
|
||||
> authenticity and creates a two-time-pad on the plaintext. `rand::random()`
|
||||
> uses the thread-local RNG which may not be a CSPRNG on all platforms; `OsRng`
|
||||
> reads from the operating system's entropy source. This task touches only
|
||||
> `encryption.rs` and can run in parallel with the irpc removal task (drift #4).
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
86
tasks/vault/poisoned-lock-recovery.md
Normal file
86
tasks/vault/poisoned-lock-recovery.md
Normal file
@@ -0,0 +1,86 @@
|
||||
---
|
||||
id: vault/poisoned-lock-recovery
|
||||
name: Replace unwrap() on RwLock acquisition with poisoned-lock recovery via unwrap_or_else
|
||||
status: pending
|
||||
depends_on: [vault/irpc-removal]
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Fix drift item #2: `VaultServiceHandle` methods use `unwrap()` on every
|
||||
`RwLock` acquisition (read and write locks). A poisoned lock (caused by a panic
|
||||
while the lock was held) would brick the vault for all subsequent operations.
|
||||
Replace with `unwrap_or_else(|e| e.into_inner())` to recover the inner data from
|
||||
a poisoned lock, or explicit error propagation where appropriate.
|
||||
|
||||
### Current state
|
||||
|
||||
`service.rs` uses `.unwrap()` on `RwLock` read and write acquisitions at
|
||||
approximately lines 142, 161, 182, 191, 196, 227, 264, 307, 340, 367 (line
|
||||
numbers may shift after the irpc removal task — match by pattern: every
|
||||
`.read().unwrap()` and `.write().unwrap()` call in `VaultServiceHandle` method
|
||||
bodies).
|
||||
|
||||
### Target state
|
||||
|
||||
For read locks:
|
||||
```rust
|
||||
let inner = self.inner.read().unwrap_or_else(|e| e.into_inner());
|
||||
```
|
||||
|
||||
For write locks:
|
||||
```rust
|
||||
let mut inner = self.inner.write().unwrap_or_else(|e| e.into_inner());
|
||||
```
|
||||
|
||||
The rationale: a poisoned lock means a panic occurred while the lock was held.
|
||||
The data may be in an inconsistent state, but bricking the vault (panicking on
|
||||
every subsequent call) is worse than attempting to continue. The vault's
|
||||
operations are idempotent reads (derive) and state transitions (lock/unlock) —
|
||||
recovering the inner data and continuing is the pragmatic choice. If the data
|
||||
is truly corrupted, the next operation will fail with a normal error, not a
|
||||
panic.
|
||||
|
||||
### No unwrap() or expect() outside tests
|
||||
|
||||
This is a general constraint for the vault: no `unwrap()` or `expect()` outside
|
||||
test code. After fixing the RwLock acquisitions, audit the rest of `service.rs`
|
||||
for any remaining `unwrap()`/`expect()` calls and replace them with proper error
|
||||
propagation (`?` operator, explicit `Result` returns, or
|
||||
`unwrap_or_else(|e| e.into_inner())` for lock recovery).
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches `service.rs` only. It depends on the irpc removal task (drift
|
||||
#4) because that task restructures `service.rs` — doing this first would cause
|
||||
merge conflicts.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] All `.read().unwrap()` calls in `VaultServiceHandle` methods replaced with `.read().unwrap_or_else(|e| e.into_inner())`
|
||||
- [ ] All `.write().unwrap()` calls in `VaultServiceHandle` methods replaced with `.write().unwrap_or_else(|e| e.into_inner())`
|
||||
- [ ] No `unwrap()` or `expect()` calls remain in `service.rs` outside of test code
|
||||
- [ ] Unit test: vault remains usable after a simulated panic (poison the lock, verify next call recovers)
|
||||
- [ ] `cargo test` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table item #2
|
||||
- docs/architecture/crates/vault/service.md — Security Constraints: No unwrap() outside tests
|
||||
- docs/architecture/decisions/025-vault-local-only-dispatch.md — ADR-025
|
||||
|
||||
## Notes
|
||||
|
||||
> A panic in one vault operation must not brick the vault for all other
|
||||
> operations. The poisoned-lock recovery via `unwrap_or_else(|e| e.into_inner())`
|
||||
> is the standard Rust pattern for this. This task depends on the irpc removal
|
||||
> task because both modify `service.rs` heavily.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
69
tasks/vault/remove-password-derivation.md
Normal file
69
tasks/vault/remove-password-derivation.md
Normal file
@@ -0,0 +1,69 @@
|
||||
---
|
||||
id: vault/remove-password-derivation
|
||||
name: Remove derive_password and site_password_path methods (password-manager pattern not relevant)
|
||||
status: pending
|
||||
depends_on: [vault/irpc-removal]
|
||||
scope: single
|
||||
risk: trivial
|
||||
impact: isolated
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Fix drift item #7: the vault currently has `derive_password`,
|
||||
`derive_password_string`, and `site_password_path` methods. These implement a
|
||||
password-manager pattern (deriving site-specific passwords from the seed) that
|
||||
is not relevant to an RPC system's vault. Remove them entirely per ADR-025
|
||||
(resolves review #002 C9).
|
||||
|
||||
### What to remove
|
||||
|
||||
- `derive_password` method from `VaultServiceHandle` (in `service.rs`)
|
||||
- `derive_password_string` method from `VaultServiceHandle` (in `service.rs`)
|
||||
- `site_password_path` function (in `mnemonic-derivation.rs` or `derivation.rs`,
|
||||
wherever it's defined)
|
||||
- Any associated path constants for password derivation
|
||||
- Any tests for these methods
|
||||
- Any references in `lib.rs` re-exports
|
||||
|
||||
### Why
|
||||
|
||||
The vault's purpose in alknet is to derive cryptographic keys (Ed25519 for
|
||||
identity, AES-256-GCM for encryption) and encrypt/decrypt external credentials.
|
||||
Site-specific password derivation is a password-manager feature that doesn't
|
||||
belong in a networking toolkit's vault. Keeping it expands the attack surface
|
||||
and API surface for no benefit.
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches `service.rs` and possibly `derivation.rs` /
|
||||
`mnemonic-derivation.rs`. It depends on the irpc removal task (drift #4) because
|
||||
both modify `service.rs`.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `derive_password` method removed from `VaultServiceHandle`
|
||||
- [ ] `derive_password_string` method removed from `VaultServiceHandle`
|
||||
- [ ] `site_password_path` function removed
|
||||
- [ ] Any password-derivation path constants removed
|
||||
- [ ] Tests for password derivation removed
|
||||
- [ ] No references to password derivation remain in `lib.rs` re-exports
|
||||
- [ ] `cargo check` succeeds (no dangling references)
|
||||
- [ ] `cargo test` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table item #7
|
||||
- docs/architecture/decisions/025-vault-local-only-dispatch.md — ADR-025 (resolves C9)
|
||||
|
||||
## Notes
|
||||
|
||||
> Straightforward removal. The password-manager pattern was inherited from the
|
||||
> POC and is not relevant to alknet's vault use case. Depends on irpc removal
|
||||
> because both modify `service.rs`.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
112
tasks/vault/review-vault-sync.md
Normal file
112
tasks/vault/review-vault-sync.md
Normal file
@@ -0,0 +1,112 @@
|
||||
---
|
||||
id: vault/review-vault-sync
|
||||
name: Review vault implementation against specs after all drift fixes
|
||||
status: pending
|
||||
depends_on: [vault/irpc-removal, vault/osrng-iv-generation, vault/poisoned-lock-recovery, vault/remove-password-derivation, vault/unlock-new-zeroizing-return, vault/key-versioning-rotation, vault/derivedkey-serialization, vault/cache-zeroization-test]
|
||||
scope: moderate
|
||||
risk: low
|
||||
impact: phase
|
||||
level: review
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Review the vault crate implementation against the architecture specs after all
|
||||
drift fixes are complete. This is the quality checkpoint before the spec-sync
|
||||
task — verify that the implementation matches the specs and that no drift
|
||||
items were missed or incompletely fixed.
|
||||
|
||||
### Review Checklist
|
||||
|
||||
1. **Drift table verification** — every item in the vault README's Known Source
|
||||
Drift table is resolved:
|
||||
- #1: OsRng for IVs (encryption.rs)
|
||||
- #2: No unwrap() on RwLock (service.rs)
|
||||
- #3: CURRENT_KEY_VERSION = 2 (encryption.rs)
|
||||
- #4: irpc removed, direct method calls (protocol.rs, service.rs, Cargo.toml)
|
||||
- #5: DerivedKey always-redact serialization (protocol.rs)
|
||||
- #6: Cache zeroization tested (cache.rs)
|
||||
- #7: derive_password removed (service.rs, derivation)
|
||||
- #8: unlock_new returns Zeroizing<String> (service.rs)
|
||||
- #9: encrypt/decrypt version-aware (service.rs)
|
||||
- #10: rotate implemented (service.rs)
|
||||
|
||||
2. **Spec conformance** — implementation matches the spec docs:
|
||||
- `VaultServiceHandle` API matches service.md (all methods, signatures, semantics)
|
||||
- `DerivedKey` / `KeyType` match protocol.md (serialization, redaction, move-only)
|
||||
- `EncryptedData` / `EncryptionKey` match encryption.md (fields, key versioning)
|
||||
- `Mnemonic` / `Seed` / `ExtendedPrivKey` match mnemonic-derivation.md
|
||||
- `KeyCache` / `CachedKey` / `CacheConfig` match service.md Cache section
|
||||
- PATHS constants match mnemonic-derivation.md (IDENTITY, DEVICE_PREFIX, SSH_HOST, ENCRYPTION, ETHEREUM)
|
||||
- `encryption_path_for_version` matches (returns InvalidPath for version < 2)
|
||||
|
||||
3. **Security constraints** (from service.md, encryption.md, README.md):
|
||||
- OsRng for IVs and salt (no `rand::random()`)
|
||||
- Zeroized drop on Seed, Mnemonic, ExtendedPrivKey, EncryptionKey, CachedKey, DerivedKey
|
||||
- No `unwrap()` or `expect()` outside tests
|
||||
- DerivedKey is move-only (no Clone)
|
||||
- DerivedKey Debug impl redacts private key
|
||||
- Cache eviction zeroizes (tested)
|
||||
- No tokio dependency (local-only, std::sync::RwLock)
|
||||
|
||||
4. **Public API** — `lib.rs` re-exports match the vault README's Public API section:
|
||||
- `Mnemonic`, `Seed`, `Language` from mnemonic
|
||||
- `DerivationError`, `ExtendedPrivKey`, `PATHS` from derivation
|
||||
- `EncryptedData`, `EncryptionError`, `EncryptionKey`, `CURRENT_KEY_VERSION` from encryption
|
||||
- `DerivedKey`, `KeyType` from protocol
|
||||
- `VaultServiceError`, `VaultServiceHandle` from service
|
||||
- `CacheConfig` from cache
|
||||
- No `VaultMessage`, `VaultProtocol`, `VaultServiceActor` (removed)
|
||||
|
||||
5. **Test coverage**:
|
||||
- Derivation test vectors (BIP39 "abandon...about" vector)
|
||||
- Encryption round-trip tests
|
||||
- Service lifecycle tests (unlock, lock, derive, encrypt, decrypt, rotate)
|
||||
- Cache tests (LRU, TTL, clear, zeroization)
|
||||
- Serialization redaction tests (JSON redact, reject redacted deserialize)
|
||||
|
||||
6. **Code quality**:
|
||||
- `cargo fmt --check` passes
|
||||
- `cargo clippy` passes with no warnings
|
||||
- No dead code (removed irpc/actor/password paths fully gone)
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] All 10 drift items verified resolved
|
||||
- [ ] VaultServiceHandle API matches service.md
|
||||
- [ ] DerivedKey / KeyType match protocol.md
|
||||
- [ ] EncryptedData / EncryptionKey match encryption.md
|
||||
- [ ] Mnemonic / Seed / ExtendedPrivKey match mnemonic-derivation.md
|
||||
- [ ] KeyCache / CachedKey / CacheConfig match service.md
|
||||
- [ ] PATHS constants match mnemonic-derivation.md
|
||||
- [ ] All security constraints satisfied (OsRng, zeroize, no unwrap, move-only, redaction)
|
||||
- [ ] Public API (lib.rs re-exports) matches vault README
|
||||
- [ ] Test coverage adequate for all functionality
|
||||
- [ ] `cargo fmt --check` passes
|
||||
- [ ] `cargo clippy` passes with no warnings
|
||||
- [ ] All tests pass
|
||||
- [ ] No dead code from removed features (irpc, actor, password derivation)
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — drift table, public API, security constraints
|
||||
- docs/architecture/crates/vault/service.md
|
||||
- docs/architecture/crates/vault/encryption.md
|
||||
- docs/architecture/crates/vault/protocol.md
|
||||
- docs/architecture/crates/vault/mnemonic-derivation.md
|
||||
- docs/architecture/decisions/018-vault-standalone-crate.md
|
||||
- docs/architecture/decisions/020-hd-derivation-for-encryption-keys.md
|
||||
- docs/architecture/decisions/021-key-rotation-via-version-indexed-paths.md
|
||||
- docs/architecture/decisions/025-vault-local-only-dispatch.md
|
||||
- docs/architecture/decisions/026-vault-key-model-hd-derivation.md
|
||||
|
||||
## Notes
|
||||
|
||||
> This review verifies the vault is spec-conformant after all drift fixes. If
|
||||
> deviations are found, document them and create fix tasks before proceeding
|
||||
> to the spec-sync task. This is the last checkpoint before the vault docs are
|
||||
> updated to remove the drift table and bump status.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
107
tasks/vault/spec-sync-remove-drift.md
Normal file
107
tasks/vault/spec-sync-remove-drift.md
Normal file
@@ -0,0 +1,107 @@
|
||||
---
|
||||
id: vault/spec-sync-remove-drift
|
||||
name: Update vault specs to remove drift table and security-constraint drift prose, bump doc status
|
||||
status: pending
|
||||
depends_on: [vault/review-vault-sync]
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
After the vault review confirms all drift is resolved, update the vault
|
||||
architecture docs to remove the drift tracking artifacts and reflect the
|
||||
completed state. The drift table and the "known drift" prose in the security
|
||||
constraints sections were tracking tools during the spec-to-implementation
|
||||
sync — now that the sync is complete, they should be cleaned up.
|
||||
|
||||
### What to update
|
||||
|
||||
1. **vault/README.md**:
|
||||
- Remove the "Known Source Drift" section (the entire table and its intro
|
||||
paragraph). The drift is resolved; the table is no longer needed.
|
||||
- Remove the "Security Constraints" drift prose — the items that said
|
||||
"The current source uses `rand::random()` — this is a known drift" etc.
|
||||
Keep the constraint statements themselves (OsRng for IVs, zeroized drop,
|
||||
no unwrap, etc.) — those are permanent implementation requirements. Remove
|
||||
only the "current source uses X, this is a known drift" sentences.
|
||||
- Bump `status: draft` → `status: stable` in the frontmatter (per the
|
||||
Document Lifecycle in the architecture README: stable = implementation
|
||||
complete and verified).
|
||||
|
||||
2. **vault/encryption.md**:
|
||||
- In Security Constraints, remove the "The current source uses
|
||||
`rand::random()` for IV generation (`encryption.rs` line 133) — this is a
|
||||
known drift from the spec and must be corrected during implementation
|
||||
sync." sentence. Keep the "OsRng for IVs" constraint.
|
||||
- In Key Versioning, remove the "The current source uses
|
||||
`CURRENT_KEY_VERSION = 1` with HD derivation and does not implement
|
||||
version-indexed paths or `rotate`. These are drift items to be corrected
|
||||
during implementation sync." paragraph.
|
||||
- Bump `status: draft` → `status: stable`.
|
||||
|
||||
3. **vault/service.md**:
|
||||
- In Security Constraints, remove the drift prose about `rand::random()`,
|
||||
`unwrap()` on RwLock, and `KeyCache::clear()` verification. Keep the
|
||||
constraint statements.
|
||||
- Bump `status: draft` → `status: stable`.
|
||||
|
||||
4. **vault/protocol.md**:
|
||||
- Remove the "to be updated per ADR-025 — remove `VaultProtocol` enum and
|
||||
irpc usage" note in References.
|
||||
- Remove the "postcard tests to be removed" note in References.
|
||||
- Bump `status: draft` → `status: stable`.
|
||||
|
||||
5. **vault/mnemonic-derivation.md**:
|
||||
- Bump `status: draft` → `status: stable` (no drift prose to remove here,
|
||||
but the doc should reflect stable status).
|
||||
|
||||
6. **architecture/README.md**:
|
||||
- Update the vault crate doc status entries in the Architecture Documents
|
||||
table from `draft` to `stable`.
|
||||
- Update the Current State paragraph to reflect vault implementation is
|
||||
complete (remove "pending ADR-025/026 refactor" language).
|
||||
|
||||
### What NOT to change
|
||||
|
||||
- Do not remove the Security Constraints sections themselves — they are
|
||||
permanent implementation requirements, not drift tracking.
|
||||
- Do not change the ADRs — they record decisions, not implementation status.
|
||||
- Do not remove the Public API section — it's a living reference.
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches only documentation files — no source code changes. It
|
||||
depends on the review task (which depends on all drift fixes).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] "Known Source Drift" table removed from vault/README.md
|
||||
- [ ] Drift prose removed from Security Constraints sections (constraint statements kept)
|
||||
- [ ] All vault doc frontmatter bumped from `status: draft` to `status: stable`
|
||||
- [ ] architecture/README.md vault doc statuses updated to `stable`
|
||||
- [ ] architecture/README.md Current State updated (no "pending refactor" language)
|
||||
- [ ] No drift-tracking language remains anywhere in vault docs
|
||||
- [ ] Security constraint statements (OsRng, zeroize, no unwrap, etc.) preserved
|
||||
- [ ] Public API section preserved in vault/README.md
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift, Security Constraints, Public API
|
||||
- docs/architecture/crates/vault/encryption.md — Security Constraints, Key Versioning
|
||||
- docs/architecture/crates/vault/service.md — Security Constraints
|
||||
- docs/architecture/crates/vault/protocol.md — References
|
||||
- docs/architecture/README.md — Document Lifecycle, Architecture Documents table, Current State
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the doc cleanup that closes out the vault phase. The drift table and
|
||||
> "known drift" prose were tracking tools during spec-to-implementation sync;
|
||||
> now that the sync is complete, they're noise. Keep the permanent constraint
|
||||
> statements — they guide future implementation agents who touch the vault.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
79
tasks/vault/unlock-new-zeroizing-return.md
Normal file
79
tasks/vault/unlock-new-zeroizing-return.md
Normal file
@@ -0,0 +1,79 @@
|
||||
---
|
||||
id: vault/unlock-new-zeroizing-return
|
||||
name: Change unlock_new return type from String to Zeroizing<String>
|
||||
status: pending
|
||||
depends_on: [vault/irpc-removal]
|
||||
scope: single
|
||||
risk: low
|
||||
impact: isolated
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Fix drift item #8: `unlock_new` currently returns `String`, which is not
|
||||
zeroized on drop. The mnemonic phrase is the root of trust — it must not linger
|
||||
in freed heap memory. Change the return type to `Zeroizing<String>` (from the
|
||||
`zeroize` crate, already a dependency).
|
||||
|
||||
### Current state
|
||||
|
||||
```rust
|
||||
pub fn unlock_new(&self, word_count: usize) -> Result<String, VaultServiceError>;
|
||||
```
|
||||
|
||||
### Target state
|
||||
|
||||
```rust
|
||||
pub fn unlock_new(&self, word_count: usize) -> Result<Zeroizing<String>, VaultServiceError>;
|
||||
```
|
||||
|
||||
Per `docs/architecture/crates/vault/service.md` → unlock_new:
|
||||
|
||||
> The returned phrase is the root of trust — it is heap-allocated and zeroized
|
||||
> on drop, so it does not linger in freed memory. The caller should extract the
|
||||
> phrase for secure storage (write down, display to user) and let the
|
||||
> `Zeroizing<String>` drop when done. Do not clone the returned value or store
|
||||
> it in a non-zeroizing container.
|
||||
|
||||
### Caller adaptation
|
||||
|
||||
The assembly layer (CLI binary, not yet implemented) will call `unlock_new` and
|
||||
extract the phrase. The `Zeroizing<String>` wrapper derefs to `String`, so
|
||||
`&*result` or `result.as_str()` works for reading. The caller must not clone the
|
||||
inner `String` into a non-zeroizing container.
|
||||
|
||||
Existing tests that call `unlock_new` need updating to handle the new return
|
||||
type — use `&*phrase` or `phrase.as_str()` to read the string.
|
||||
|
||||
### Scope
|
||||
|
||||
This task touches `service.rs` (the method signature and body) and test files.
|
||||
It depends on the irpc removal task (drift #4) because both modify `service.rs`.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `unlock_new` return type changed from `Result<String, ...>` to `Result<Zeroizing<String>, ...>`
|
||||
- [ ] Method body constructs `Zeroizing<String>` from the generated phrase
|
||||
- [ ] Existing tests updated to handle `Zeroizing<String>` return type
|
||||
- [ ] No `clone()` of the returned value in non-test code
|
||||
- [ ] `cargo check` succeeds
|
||||
- [ ] `cargo test` succeeds
|
||||
- [ ] `cargo clippy` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/vault/README.md — Known Source Drift table item #8
|
||||
- docs/architecture/crates/vault/service.md — unlock_new section
|
||||
- docs/architecture/decisions/025-vault-local-only-dispatch.md — ADR-025 (resolves W7)
|
||||
|
||||
## Notes
|
||||
|
||||
> The mnemonic is the root of trust. Returning a plain `String` means the phrase
|
||||
> lingers in freed heap memory after the caller drops it. `Zeroizing<String>`
|
||||
> zeroizes the bytes on drop. This resolves review #002 W7. Depends on irpc
|
||||
> removal because both modify `service.rs`.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
Reference in New Issue
Block a user