feat(trust): adding digital signature verification on import#237
feat(trust): adding digital signature verification on import#237Akshat-Raj wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to shift soul import verification to a fail-closed posture by running trust-chain verification during Soul.awaken(), raising a SoulTrustError on verification failure unless explicitly bypassed.
Changes:
- Introduces
VerificationState/VerificationResultand updates trust-chain verification APIs to return structured results. - Wires trust-chain verification into
Soul.awaken()with anallow_unverifiedbypass path (logs a warning instead of raising). - Adds import-path tests for valid chains and several tampering scenarios; updates CLI
verify/auditto awaken in allow-unverified mode so they can inspect invalid souls.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_trust_chain/test_import_verification.py | Adds tests asserting awaken passes for valid chains and fails (or warns) for tampered/missing-signature/key-mismatch chains. |
| src/soul_protocol/runtime/trust/manager.py | Adds VerificationState/VerificationResult and changes TrustChainManager.verify() to return structured verification info. |
| src/soul_protocol/runtime/soul.py | Adds allow_unverified to Soul.awaken() and enforces trust-chain verification on load, raising SoulTrustError on failures. |
| src/soul_protocol/runtime/exceptions.py | Introduces SoulTrustError for trust verification failures. |
| src/soul_protocol/cli/main.py | Updates verify and audit commands to awaken with allow_unverified=True so they can operate on invalid souls. |
Comments suppressed due to low confidence (1)
src/soul_protocol/runtime/soul.py:890
allow_unverifiedis added to the method signature, but the docstring’s Args section doesn’t mention it. Please document what it does (bypass fail-closed import and emit a warning) so API callers discover it without reading the implementation.
async def awaken(
cls,
source: str | Path | bytes,
engine: CognitiveEngine | Callable | str | None = None,
search_strategy: SearchStrategy | None = None,
password: str | None = None,
eternal: EternalStorageManager | None = None,
allow_unverified: bool = False,
) -> Soul:
"""Awaken a Soul from a .soul file, directory, soul.json, soul.yaml, or soul.md.
Args:
source: Path to soul file/directory, or raw bytes of a .soul archive.
Directories must contain a ``soul.json`` file.
engine: Optional CognitiveEngine for LLM-enhanced cognition.
search_strategy: Optional SearchStrategy for pluggable retrieval (v0.2.2).
password: Optional password for decrypting encrypted .soul archives.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Akshat-Raj
changed the title
| if verification_result.status == VerificationState.FAILED: | ||
| if allow_unverified: | ||
| logger.warning( | ||
| "Awakening unverified soul. Signer: %s, Reason: %s", | ||
| verification_result.signer, | ||
| verification_result.reason, | ||
| ) | ||
| else: | ||
| logger.error("Soul import refused due to trust chain failure.") | ||
| raise SoulTrustError( | ||
| f"Refusing to awaken soul: {verification_result.reason}. " | ||
| "Pass allow_unverified=True to override." | ||
| ) | ||
| elif verification_result.status == VerificationState.WARNED: | ||
| logger.warning(f"Trust chain warning: {verification_result.reason}") |
| return VerificationResult( | ||
| status=VerificationState.FAILED, | ||
| reason=f"public key mismatch at seq {entry.seq}", | ||
| signer=entry.public_key, |
| logger.error("Soul import refused due to trust chain failure.") | ||
| raise SoulTrustError( | ||
| f"Refusing to awaken soul: {verification_result.reason}. " | ||
| "Pass allow_unverified=True to override." |
| from soul_protocol.runtime.soul import Soul | ||
| from soul_protocol.spec.trust import chain_integrity_check | ||
|
|
||
| soul = await Soul.awaken(path) | ||
| soul = await Soul.awaken(path, allow_unverified=True) | ||
| summary = chain_integrity_check(soul.trust_chain) |
| async def _audit(): | ||
| from soul_protocol.runtime.soul import Soul | ||
|
|
||
| soul = await Soul.awaken(path) | ||
| soul = await Soul.awaken(path, allow_unverified=True) | ||
| log = soul.audit_log(action_prefix=action_prefix, limit=limit) |
prakashUXtech
left a comment
prakashUXtech
left a comment
There was a problem hiding this comment.
The architecture matches the spec (structured VerificationResult, three-state enum, --allow-unverified bypass with WARNING log), and the test matrix exercises all five acceptance scenarios from #227. Couple of blockers and a handful of polish items before it can land.
Blockers
1. Retarget to dev, not main.
Same workflow miss as your #232. All v0.5.0 work goes through dev per tracker #218. Rebase + update the PR base.
2. Legacy souls (empty chain) shouldn't be FAILED.
TrustChainManager.verify() now returns VerificationState.FAILED when chain.entries is empty (trust/manager.py). The test test_legacy_archive_without_chain_still_loads was updated to expect FAILED and pass allow_unverified=True to make it load.
The issue says "refuse to import on hard failure", but a pre-trust-chain legacy soul isn't tampered, it's just from before the trust chain existed. Conflating "no chain" with "broken chain" means anyone with a v0.3.x soul archive has to use --allow-unverified forever to load it. That defeats the security posture: users train themselves to always pass the flag, then miss real tampering.
Suggested fix: map empty chain to WARNED instead of FAILED. That logs a heads-up about a legacy file but still loads. Keep FAILED for actually-broken chains (tampered signatures, key mismatches).
High-priority cleanup
3. The VerificationResult tuple shim deserves a deprecation path.
__iter__ + __eq__ against tuples let passed, reason = result and result == (True, None) keep working for old callers. Two issues with the shim as it stands:
- No removal path. It'll be in the API forever unless someone files a follow-up issue.
- The
__eq__against tuples is asymmetric.result == (True, None)works (callsVerificationResult.__eq__first), but(True, None) == resultcalls Python's tuple__eq__first, getsNotImplemented, and falls back. Equality should be symmetric.
Either grep for the few tuple-unpacking callers (should be under 10 sites) and migrate them now, or leave the shim with a TODO: remove in 0.6.0 (see #XXX) comment pointing at a follow-up issue.
Polish
- Branch name
cliis too generic. Future you (or whoever rebases on top) will thank you forfeat/issue-227-trust-import-verificationinstead. - Thinking-out-loud comments in
test_import_verification.py:81-83. The "Need to update payload hash..." → "Wait..." → "So it should..." three-line train-of-thought is great while writing the test, but should be cleaned up to a clear one-line docstring before posting. Worth a self-check pass before the next push. tests/test_cli/test_org_init.pyhas an unrelated whitespace change (removed blank line between two tests, line 95). Revert to keep the diff focused on this issue.SoulTrustError.__str__drops the signer. The exception capturessigneras an attribute, but the message format isf"Trust chain validation failed: {reason}". Operators triaging from logs would benefit from including signer in the message too.- CHANGELOG entry missing. This is both an addition (the verification +
SoulTrustError) and a behavior change (default refuses tampered souls). Add lines under both## Addedand## Changed. --allow-unverifiedis only onverifyandauditcommands. Other CLI entry points that load souls (inspect,status,recall, etc.) will hard-refuse on a broken soul with no escape hatch. Intentional? If so, doc it (verify/audit are the diagnostic paths, others enforce strict). If not, add the flag.
What's working
- The structured
VerificationResultdataclass + three-state enum matches the issue spec exactly - Test matrix is the right one (clean / tampered / missing / key-rotation / bypass-warns)
- Adding
lifecycle.birthchain entry inbirth()andlifecycle.reincarnateinreincarnate()is a thoughtful touch: removes the edge case of "freshly-birthed has no chain" - Honest test updates (
test_encryption.py,test_back_compat_load.py,test_persistence.pypassingallow_unverified=Trueexplicitly). The dependency is visible rather than hidden. - New
SoulTrustErrorwithreason+signerfields makes downstream error handling cleaner
Once the base retarget + the legacy-souls semantics + the tuple-shim path are sorted, this is mergeable.
|
@prakashUXtech Will fix the issues ASAP! |
What does this PR do?
This PR shifts the trust chain verification from a "fail-open" to a "fail-closed" security posture. It wires trust chain verification directly into the Soul.awaken() import path. If a .soul file has a tampered, missing, or broken trust chain, it will now hard-fail and refuse to load by raising a SoulTrustError. This strict verification can be bypassed using the newly introduced --allow-unverified flag, which will emit a WARNING log instead.
Fixes #227
How to test
Checklist
uv run pytest tests/)uv run ruff check . && uv run ruff format --check .)feat:,fix:,docs:, etc.)