feat: initial dig-session facade (unlock/enroll/sign/inject) - #1
Conversation
Introduce the DIG session/keystore layer: a custody-safe facade composing dig-keystore (encrypted storage) + dig-identity (canonical BLS derivation). - Session::unlock<K> — generic load -> unlock -> SignerHandle - Session::enroll_identity — derive the canonical identity key once and store it under L1WalletBls (from_bytes round-trip), NOT BlsSigning (from_seed would re-derive a different key — dig_ecosystem #64/#57) - UnlockedIdentity — zeroizing, redacting Debug, no Clone; sign + inject a bare SigningFn primitive so consumers stay identity-agnostic (#908) No seal/decap (belongs to dig-message). SPEC.md is the normative contract. Full CI gate set + release/publish workflows from commit 1. Closes #1249 Co-Authored-By: Claude <noreply@anthropic.com>
The org has GitHub Advanced Security default setup enabled (as do sibling crates dig-keystore/dig-identity, which ship no codeql.yml). An advanced CodeQL config cannot upload SARIF while default setup is enabled, so the custom workflow failed. Remove it and let default setup provide CodeQL. Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CORRECTNESS GATE — PASS (custody-adjacent, independent verify)
Verdict: PASS. Reviewed against #1249 acceptance criteria, §2.5 readable-code, §5.1 zeroize/custody, #908 identity-agnostic boundary. All 4 required checks green (version-increment, coverage 98.18% >=80%, commitlint, test suite); zero open review threads.
L1WalletBls scheme choice — CORRECT (verified against dig-keystore 0.4.1 source)
L1WalletBls::secret_to_secret_keyusesSecretKey::from_bytes(l1_wallet_bls.rs:87-98) — a faithful round-trip of the exact derived scalar. Enrollment storesderive_identity_sk(master).to_bytes()(session.rs:72-74) and unlock reconstructs it byte-identically.BlsSigning::secret_to_bls_secret_keyrunsSecretKey::from_seed(bls_signing.rs:51-61) — storing the already-derived key there would re-derive a DIFFERENT key. The chosen scheme avoids exactly the #64/#57 double-derivation bug.- Regression test C-2 (
enrolled_public_key_matches_dig_identity_canonical, tests/facade.rs:41-63) computes the expected pubkey INDEPENDENTLY viadig_identity::public_key_bytes(derive_identity_sk(master))and asserts equality — it genuinely proves parity and WOULD fail under a revert toBlsSigning. Confirmed real, not vanity.
Zeroize delegation — SUFFICIENT, marker trait NOT needed
- The only secret material lives in
SignerHandle'sZeroizing<Vec<u8>>(signer.rs:19-23), wiped on drop.UnlockedIdentitywraps that handle + aPublicKey(non-secret) — no independent secret buffer to zeroize, so#[derive(ZeroizeOnDrop)]would add nothing. signing_fnclones theSignerHandleinto theArcclosure (unlocked.rs:67-70); the clone is itself aZeroizingbuffer wiped when the closure drops. Enrollment's transientZeroizingsecret (session.rs:74) is also wiped.- No
CloneonUnlockedIdentity;Debugredacts secret AND public key (unlocked.rs:85-92, test C-8). Delegation is the correct design here.
API boundary (#908) — no identity type leaks
SigningFn<K> = Arc<dyn Fn(&[u8]) -> K::Signature + Send + Sync>(unlocked.rs:16).K::Signatureresolves tochia_bls::Signature— a primitive.inject_intohands a consumer only the closure. Confirmed no dig-session/dig-identity type crosses the boundary.
Other gates
- Reference-down legal (L10 -> L00 foundation); crates.io-only deps (dig-keystore 0.4.1, dig-identity 0.4.2, verified in Cargo.lock); chia-bls unified at 0.26.0 across the graph (no dual-version type mismatch on the
dig_keystore::blsre-exports). - GPL-2.0-only correct (transitively links GPL dig-identity). SPEC.md normative + matches code. release.yml + publish.yml both carry
workflow_dispatch(new-crate first-release race covered). readable-code: intent-revealing, WHY-comments present, small single-purpose modules.
Non-gating nit (does NOT block merge)
chia-bls = "0.26"is declared as a direct dependency in Cargo.toml:34 but is never referenced insrc/(only in doc-comment prose); the BLS types are re-exported fromdig_keystore::bls. Harmless (resolves to the same 0.26.0 already in-tree) but it is an unused direct dep — safe to drop in a follow-up. Not a blocker.
No approval event posted (self-approval 422); this PASS review + zero unresolved threads unblocks the orchestrator's squash-merge once branch protection shows all-green.
HOLD — merge deferred pending final gate leg + harness reconciliationStatus: NOT merged. dig-session is custody-critical (session unlock/sign for the DID-anchored identity key), so it merges only on a complete, real (non-glitched) gate set. Gate state on this HEAD (
Resume condition: the security-audit leg returns a real (non-glitched) verdict on Context: a harness-level env-glitch (some fresh gate contexts returning "No tools needed"/context-mode MCP "No such tool") is tracked in #1324. It did NOT affect this PR's correctness or adversarial legs (both executed tools fully). PR branch + HEAD are durable; no work is at risk. |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CORRECTNESS GATE — PASS
Fresh-context correctness review of the new dig-session crate (L10 seam-7, custody-adjacent). All checklist items clear.
1. L1WalletBls scheme choice — CORRECT (custody-critical)
Verified against the actual dependency source, not just the docs:
dig-keystoreL1WalletBls::secret_to_secret_keyreconstructs viaSecretKey::from_bytes(scheme/l1_wallet_bls.rs:97) — exact round-trip of the stored scalar.BlsSigningreconstructs viaSecretKey::from_seed(scheme/bls_signing.rs:60) — would re-derive a different key from the same bytes.Keystore::create(Some(secret), …)stores the provided bytes verbatim after a== SECRET_LEN(32)check (keystore.rs:146-157) — no re-derivation on the write path.enroll_identity(src/session.rs:72-85) derivesderive_identity_sk(master_secret_key_from_seed(seed))once, storesidentity_sk.to_bytes()(a canonical in-range BLS scalar, sofrom_bytesaccepts it) underL1WalletBls.
Storing the already-derived key under L1WalletBls (round-trip) rather than the locked design's BlsSigning (one-way from_seed) is the correct deviation — BlsSigning would silently yield a key whose pubkey does not match the DID-anchored identity. Documented at src/lib.rs:22-38 and SPEC.md:59-64.
2. C-2 regression is genuine, not tautological
tests/facade.rs:41-63 computes expected independently via dig_identity::public_key_bytes(derive_identity_sk(master_secret_key_from_seed(SEED))) and asserts the enrolled pubkey equals it. Reverting storage to BlsSigning would re-derive on unlock and this assertion would fail. Real tripwire.
3. inject_into / SigningFn — identity-agnostic (#908) confirmed
SigningFn<K> = Arc<dyn Fn(&[u8]) -> K::Signature + Send + Sync> (src/unlocked.rs:16). K::Signature resolves to the primitive chia_bls::Signature; no dig-session/dig-identity type appears in the injected signature. Consumers stay identity-agnostic.
4. Zeroize delegation — SUFFICIENT
UnlockedIdentity wraps SignerHandle<K> (secret in Zeroizing<Vec<u8>>, wiped on drop — dig-keystore/src/signer.rs:19-23) plus a non-secret PublicKey. No Clone on UnlockedIdentity; signing_fn clones the SignerHandle into an Arc closure, each buffer independently zeroized on last-drop; Debug redacts (src/unlocked.rs:85-92, test C-8). A literal #[derive(ZeroizeOnDrop)] is unnecessary because the only secret-bearing field self-zeroizes via delegation. Correct.
5. Layering / deps / license
Reference-down only (L10 → L00 dig-keystore 0.4.1, dig-identity 0.4.2), crates.io versions, no git deps, #![forbid(unsafe_code)], GPL-2.0-only. Correct.
6. Tests / coverage / SPEC / CI
C-1..C-8 all present and real (tests/facade.rs). CI green: fmt, clippy -D warnings, nextest, doctests, cargo doc, coverage gated >=80% (reported 98.18%). Branch protection: 0 required approvals, strict=true, required_conversation_resolution=true, required-check names match. release.yml/publish.yml carry workflow_dispatch. GHAS default setup (no custom codeql.yml) with least-privilege permissions: contents: read. 0 open review threads.
Non-gating notes (no action required)
Cargo.tomlenableszeroize'sderivefeature but the crate uses onlyZeroizing(no#[derive(Zeroize)]). Harmless unused feature.identity_sk.to_bytes()yields a transient[u8;32]on the stack before theZeroizingVeccopy; the array copy isn't explicitly zeroized (chia-blsSecretKeyitself zeroizes on drop). Defense-in-depth only.
Verdict: PASS. Merge remains the orchestrator's action (checks green, zero unresolved threads).
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Correctness gate (fresh context, custody-adjacent L10 session crate). VERDICT: CHANGES-REQUIRED — one gating finding on the stated zeroize criterion (item 2); everything else verified green.
VERIFIED GREEN:
- Custody round-trip (item 1): enroll stores identity_sk.to_bytes(); L1WalletBls reconstructs via SecretKey::from_bytes (faithful), NOT from_seed. Confirmed against dig-keystore 0.4.1 source (l1_wallet_bls.rs from_bytes; bls_signing.rs from_seed). C-2 test asserts equality vs public_key_bytes(derive_identity_sk(master)) and genuinely fails if reverted to BlsSigning. 8 tests + doctest pass locally.
- API shape (item 3): SigningFn leaks no key type; inject_into hands only the closure; #908 boundary held.
- Derivation (item 4): m/12381'/8444'/9'/0' via dig_identity::derive_identity_sk; verified by the canonical-match test.
- No Clone on UnlockedIdentity; Debug redacts secret + omits pubkey.
- Reference-down: deps dig-keystore 0.4.1 / dig-identity 0.4.2 / chia-bls / zeroize / thiserror, all crates.io, no git dep. GPL-2.0-only. Branch protection strict / 0 approvals / conversation-resolution + linear. 4 required checks green. 0 pre-existing threads.
|
|
||
| // Derive the canonical identity signing key ONCE. | ||
| let master = master_secret_key_from_seed(seed); | ||
| let identity_sk = derive_identity_sk(&master); |
There was a problem hiding this comment.
GATING (item-2 zeroize): the master and identity_sk locals are chia_bls::SecretKey, which in chia-bls 0.26 is #[derive(PartialEq, Eq, Clone)] over a blst_scalar with NO Zeroize/Drop impl. Both hold the raw identity signing scalar and are dropped WITHOUT wiping when enroll_identity returns, leaving key material in freed stack memory. Only the extracted secret bytes (line 74) are wrapped in Zeroizing, so the stated criterion 'enroll ... derived master sk zeroized on all paths' is not met, and the Cargo.toml claim 'zeroize derived secrets on drop' is only partially delivered.
Resolve by either: (a) minimizing/wiping — e.g. derive into the bytes and explicitly overwrite the SecretKey memory via a helper, or gate on an upstream chia-bls zeroize-capable version; or (b) if wiping an un-Zeroize-able upstream type is genuinely infeasible at 0.26, document the residual exposure explicitly in the enroll doc-comment + DEVELOPMENT_LOG and narrow the Cargo.toml claim. loop-security/adversarial run separately and own the final custody call on severity.
Task
New crate dig-session (#1249, seam-7 session/keystore extraction) — the DIG session/keystore layer: unlock identity keys + inject signing primitives. Composes
dig-keystore+dig-identitybehind one curated, custody-safe facade. GPL-2.0-only. Tracks super-repo issueDIG-Network/dig_ecosystem#1249.Public API
Session::unlock::<K: KeyScheme>(backend, path, password) -> Result<UnlockedIdentity<K>>— genericload -> unlock -> SignerHandle.Session::enroll_identity(backend, path, password, seed) -> Result<UnlockedIdentity<L1WalletBls>>— derive the canonical dig-identity key once, store it, return it unlocked.UnlockedIdentity<K>:public_key,sign,signing_fn,inject_into(yields a bareSigningFnprimitive so consumers stay identity-agnostic, #908). Zeroizing, redactingDebug, noClone.SigningFn<K> = Arc<dyn Fn(&[u8]) -> K::Signature + Send + Sync>.SessionError/Result.Deviation from the locked design (custody-critical) — needs gate attention
The locked design said "store the DERIVED sk via
Keystore::createunder BlsSigning". That is INCORRECT:BlsSigningreconstructs its stored bytes viachia_bls::SecretKey::from_seed(a one-way KDF) on every unlock, so storing an already-derived identity sk underBlsSigningre-derives a DIFFERENT key (the documented #64/#57 pitfall) whose public key would NOT match the DID-anchored identity key.Resolution: the identity path stores the derived sk under
L1WalletBls(thefrom_bytesfaithful round-trip scheme).Session::unlockstays generic, soBlsSigningis still available for seed-derived validator keys. A regression test (C-2) asserts the enrolled public key equalsdig_identity::public_key_bytes(derive_identity_sk(master)), and would fail on any revert toBlsSigning.Scope
No seal/decap (belongs to dig-message, same crate level). First cut = unlock/sign/inject only.
Verification
cargo test: 8 integration tests + 1 doctest green.cargo fmt --check: clean.cargo clippy --all-targets -D warnings: clean.cargo llvm-cov: 98.18% lines / 96.34% regions (floor 80%).Blast radius
New standalone crate, zero existing callers. Not yet a superproject submodule (main adds it after publish). No cross-repo symbol edits.
Draft until CI green; do NOT merge (awaiting triple gate). Do NOT publish to crates.io yet.