fix(store): fail-closed on sign_manifest failure — prevent install-gate bypass#2050
fix(store): fail-closed on sign_manifest failure — prevent install-gate bypass#2050hognek wants to merge 10 commits into
Conversation
…l-v2 Add code signing to the app store install flow (jaylfc#647): - store_signing.py: Ed25519 keypair generation, persistence, sign/verify utilities that mirror the hub/identity.py pattern - registry.py: AppRegistry now accepts a signing key, signs every manifest at catalog load time, and exposes verify_manifest_signature() for re-verification at install time - store_install.py: signature verification gate in install-v2 before any installer or script runs; tampered manifests are rejected with 403 - GET /api/store/signing-pubkey: public key endpoint for clients and auditing tools - app.py: loads/creates the store signing keypair on boot Design: one Ed25519 keypair per taOS instance, generated on first boot, private key never leaves the node. Signatures are computed at catalog load time; install-v2 re-verifies against the stored signature to detect post-boot catalog tampering. Tests: 13 unique unit tests covering keypair lifecycle, sign/verify, tamper detection, deterministic signatures, _signature field stripping, and file permissions.
- registry.py: wrap sign_manifest() in try/except so a malformed signing key does not crash the entire catalog load; add logging import - app.py: wrap load_or_create_signing_keypair() in try/except OSError so a read-only data_dir does not prevent server startup - store_signing.py: enforce 0600 permissions on existing keyfile load (from round 1) - store_install.py: document threat model limitation; change pubkey endpoint 500→404 when unconfigured - tests: update pubkey endpoint test to expect 404
- store_signing.py: atomic keyfile creation with O_CREAT|O_EXCL+0o600, derive public key from loaded private key instead of trusting data['public_pem'], handle FileExistsError race by loading winner - registry.py: verify_manifest_signature now re-reads manifest.yaml from disk at install time, so post-boot catalog tampering is actually detected (previously compared two in-memory copies loaded at boot) - store_install.py: add _verify_manifest_for_install helper that distinguishes 'no stored signature' (graceful skip) from 'bad signature' (hard 403); verify backend manifests in install chain before running their installer - test_store_signing.py: fix flaky sig[:-2]+'ff' to XOR last byte so it always differs from original (~1/256 failure fixed) - test_routes_store_install.py: add end-to-end real-registry tamper test that mutates on-disk YAML and asserts 403 Tests: 13/13 store_signing pass. Install route fixture has pre-existing aiosqlite hang (CI will cover).
…e redundant os import - FileExistsError fallback now uses a PID-unique tmp path on retry so it never collides with a still-running winner's tmp file. - Added an early keyfile.exists() check after the 3s wait (the competing process may have completed os.replace by then). - Removed local 'import os as _os_module' and 'import time as _time' — os was already a top-level import and time is now imported at module level. - Replaced all _os_module.* and _time.* references with os.* and time.* respectively. Fixes: Kilo WARNING (store_signing.py:153) + SUGGESTION (line 132)
…1924 1. store_signing.py: enforce 0o600 BEFORE reading keyfile (umask bypass) 2. registry.py: fail-open for unsigned manifests, document policy, add set_signing_key() 3. store_install.py: TOCTOU guard — re-verify manifest from disk at execution time 4. app.py: defer keypair loading to lifespan (read-only data_dir no longer bricks startup) 5. store_install.py: 500→422 for backend without installer mapping Also fix test_real_registry_detects_post_load_tampering empty installed_path init.
…ecovery Skip the shared tmp name entirely after a FileExistsError contention timeout instead of unlinking it. Unlinking the shared tmp could race with a still-running winner process, causing a FileNotFoundError crash for the winner or inconsistent on-disk state. Jump directly to the unique PID-based tmp path, which is safe because every process gets its own name.
…_signature + second signature re-verify TOCTOU guard - registry.py: Change verify_manifest_signature from fail-open to fail-closed (returns False for unsigned manifests). The install gate (_verify_manifest_for_install) already short-circuits for unsigned manifests via get_signature() check, so the install path is unaffected. This prevents future callers from accidentally allowing unsigned manifests through. - store_install.py: Replace the TOCTOU field-comparison guard with a second signature re-verify against the re-read disk bytes. This is more robust: catches any change (not just the whitelisted fields), does not false-positive on legitimate catalog reloads, and aligns with the existing Ed25519 trust model. Fixes the 2 remaining Kilo SUGGESTIONS on jaylfc#2023.
PR jaylfc#2023 changed the _BACKEND_TO_METHOD lookup failure from 500 to 422. Update both test_unknown_backend_returns_500 assertions and docstrings: - tests/routes/test_store_install_v2.py::TestBackendToMethodMapping - tests/test_routes_store_install.py::TestInstallV2
…te bypass When sign_manifest raised, the exception was logged but no signature was stored. _verify_manifest_for_install saw None and short-circuited to (True, None) — allowing install with zero tamper protection for any manifest whose signing threw. An attacker inducing a signing failure for a target manifest could bypass the Ed25519 gate silently. Track signing failures separately in AppRegistry._signing_failures and expose had_signing_failure(). In _verify_manifest_for_install, check for a signing failure before the unsigned short-circuit, and block the install with a clear message when signing failed. Kilo: failures silently downgrade to unsigned Ref: PR jaylfc#2027 review comment on tinyagentos/registry.py:141
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds persistent Ed25519 signing for store manifests, registry-level signature verification, install-time and TOCTOU checks, a signing public-key endpoint, and HTTP 422 handling for unsupported backends. ChangesStore signing and installation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant InstallRoute
participant AppRegistry
participant ManifestFile
Client->>InstallRoute: POST /api/store/install-v2
InstallRoute->>AppRegistry: verify_manifest_signature
AppRegistry->>ManifestFile: read manifest.yaml
ManifestFile-->>AppRegistry: current manifest data
AppRegistry-->>InstallRoute: verification result
InstallRoute-->>Client: 403 on failure or continue installation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| "failed to sign manifest %s — catalog load continues unsigned", | ||
| catalog[-1].id, | ||
| ) | ||
| signing_failures.add(catalog[-1].id) |
There was a problem hiding this comment.
WARNING: Fail-closed signing failure can block valid manifests with date/timestamp fields
yaml.safe_load parses ISO date/timestamp strings (e.g. release_date: 2024-01-01) into datetime.date/datetime.datetime objects. _canonical_manifest_bytes then calls json.dumps(stripped, ...) (store_signing.py:209) with no default=, which raises TypeError on those values. That TypeError is swallowed by this broad except Exception: and the manifest is recorded in signing_failures. Under the new fail-closed policy (had_signing_failure → 403), any legitimate manifest containing a date field becomes permanently uninstallable (until restart), even though it is not tampered.
This turns a latent serialisation limitation into a hard availability regression. Consider making canonicalisation YAML-safe, e.g. json.dumps(stripped, ..., default=str) or re-encoding through yaml.safe_dump, so non-malicious values don't trip the fail-closed path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| disk_path = _manifest_dir / "manifest.yaml" | ||
| try: | ||
| import yaml as _yaml | ||
| on_disk = _yaml.safe_load(disk_path.read_text()) if disk_path.exists() else None |
There was a problem hiding this comment.
SUGGESTION: Synchronous blocking I/O + crypto inside the async route
This TOCTOU re-read (disk_path.read_text() + yaml.safe_load + Ed25519 verify_manifest_signature) runs directly on the event loop (it is not awaited or offloaded to a thread). For an on-disk catalog this is usually fast, but under load or slow/network filesystems it blocks all concurrent requests for the duration of the disk read and verification.
Consider wrapping the read+verify in await asyncio.to_thread(...) (or reusing a threadpool) to keep the event loop responsive, consistent with how other blocking store operations in this app are handled.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Both issues from the previous review have been resolved in the latest commits:
The incremental changes preserve the original gate logic and introduce no new issues. Files Reviewed (incremental — 2 changed files)
Previous Review Summary (commit b451956)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b451956)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by hy3:free · Input: 40.7K · Output: 3.4K · Cached: 123K |
…ion + async TOCTOU - _canonical_manifest_bytes: add default=str to json.dumps so yaml.safe_load-produced date/datetime values don't cause signing failures on legitimate manifests (Kilo WARNING, registry.py:152) - TOCTOU re-verify: wrap disk read + Ed25519 verify in asyncio.to_thread to avoid blocking the event loop under load (Kilo SUGGESTION, store_install.py:796)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tinyagentos/routes/store_install.py (1)
758-776: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrimary signing gate still does blocking disk I/O + Ed25519 verify on the event loop.
The TOCTOU re-verify below (Line 812) is correctly offloaded via
asyncio.to_thread, but this primary gate calls_verify_manifest_for_installsynchronously, andregistry.verify_manifest_signaturere-readsmanifest.yamlfrom disk and runs Ed25519 verification inline. For a local on-disk catalog this is fast, but under concurrency or on slow/network filesystems it blocks all requests. Offload it for consistency with the TOCTOU path.♻️ Offload the primary gate
- verified, verify_err = _verify_manifest_for_install( - manifest_id, registry, _store_pub, - ) + verified, verify_err = await asyncio.to_thread( + _verify_manifest_for_install, manifest_id, registry, _store_pub, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/store_install.py` around lines 758 - 776, Offload the primary manifest verification call in the install route to a worker thread, matching the existing asynchronous TOCTOU verification path. Update the call to _verify_manifest_for_install to use asyncio.to_thread while preserving its arguments and the existing failure response handling.tests/test_routes_store_install.py (1)
340-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test names after the 500→422 change. The unknown-backend response is now HTTP 422, but both test method names still say
500.
tests/test_routes_store_install.py#L340-L357: renametest_unknown_backend_returns_500→test_unknown_backend_returns_422.tests/routes/test_store_install_v2.py#L180-L227: renametest_unknown_backend_returns_500_not_exception→ reflect 422.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_routes_store_install.py` around lines 340 - 357, Rename the unknown-backend test in tests/test_routes_store_install.py:340-357 from test_unknown_backend_returns_500 to test_unknown_backend_returns_422. Also rename test_unknown_backend_returns_500_not_exception in tests/routes/test_store_install_v2.py:180-227 to a name reflecting the 422 response; leave the test assertions and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/app.py`:
- Around line 1597-1613: Move the store signing keypair initialization and
registry.set_signing_key() call out of create_app() and into the application
lifespan startup path. Ensure create_app() only prepares the relevant state,
while key loading and catalog reload occur lazily during lifespan startup;
update the comments to match the resulting behavior.
---
Nitpick comments:
In `@tests/test_routes_store_install.py`:
- Around line 340-357: Rename the unknown-backend test in
tests/test_routes_store_install.py:340-357 from test_unknown_backend_returns_500
to test_unknown_backend_returns_422. Also rename
test_unknown_backend_returns_500_not_exception in
tests/routes/test_store_install_v2.py:180-227 to a name reflecting the 422
response; leave the test assertions and behavior unchanged.
In `@tinyagentos/routes/store_install.py`:
- Around line 758-776: Offload the primary manifest verification call in the
install route to a worker thread, matching the existing asynchronous TOCTOU
verification path. Update the call to _verify_manifest_for_install to use
asyncio.to_thread while preserving its arguments and the existing failure
response handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 830833b2-14e6-44d5-8f3e-9d58eb7549f2
📒 Files selected for processing (7)
tests/routes/test_store_install_v2.pytests/test_routes_store_install.pytests/test_store_signing.pytinyagentos/app.pytinyagentos/registry.pytinyagentos/routes/store_install.pytinyagentos/store_signing.py
| # Load the store signing keypair lazily here in the lifespan, not in | ||
| # create_app(), so a read-only data_dir does not brick startup. | ||
| # When the keypair cannot be loaded (missing cryptography, unwritable | ||
| # data_dir), signing is simply disabled — the install gate falls | ||
| # through to unsigned (fail-open) and the pubkey endpoint returns 404. | ||
| _store_pub: bytes | None = None | ||
| try: | ||
| _store_priv, _store_pub = load_or_create_signing_keypair(data_dir) | ||
| if _store_priv is not None: | ||
| registry.set_signing_key(_store_priv) | ||
| except OSError: | ||
| logger.warning( | ||
| "store signing keypair could not be created (data_dir=%s may be " | ||
| "read-only) — catalog signatures will not be available", | ||
| data_dir, | ||
| ) | ||
| app.state.store_signing_pubkey = _store_pub |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the signing-key init is only in the eager create_app body and not duplicated in the lifespan,
# and confirm set_signing_key triggers an eager catalog reload.
rg -nP -C3 'set_signing_key|store_signing_pubkey|load_or_create_signing_keypair' tinyagentos/app.py
rg -nP -C3 'def reload|def _load_catalog|def set_signing_key' tinyagentos/registry.pyRepository: jaylfc/taOS
Length of output: 2722
🏁 Script executed:
#!/bin/bash
sed -n '1500,1620p' tinyagentos/app.py
printf '\n--- registry ---\n'
sed -n '100,175p' tinyagentos/registry.pyRepository: jaylfc/taOS
Length of output: 9774
Move this block into the lifespan, or fix the comments
It still runs in create_app(), so the deferred startup path never takes effect. registry.set_signing_key() also reloads the catalog immediately, making startup pay the manifest walk/parse up front.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/app.py` around lines 1597 - 1613, Move the store signing keypair
initialization and registry.set_signing_key() call out of create_app() and into
the application lifespan startup path. Ensure create_app() only prepares the
relevant state, while key loading and catalog reload occur lazily during
lifespan startup; update the comments to match the resulting behavior.
1 participant
Summary
Fix Kilo WARNING from PR #2027: When
sign_manifestraises, the exception was logged but no signature was stored._verify_manifest_for_installsawNoneand short-circuited to(True, None)— allowing install with zero tamper protection for any manifest whose signing threw. An attacker inducing a signing failure for a target manifest could bypass the Ed25519 gate silently.Changes
registry.py: Added_signing_failures: set[str]tracking +had_signing_failure()method. Whensign_manifestraises during catalog load, the app_id is recorded in the failure set.store_install.py: In_verify_manifest_for_install, whenstored_sig is None, checkhad_signing_failure()first and return(False, msg)if signing failed — block the install gate rather than silently allowing it.Tests
Reference
Kilo:
sign_manifestfailures silently downgrade to unsigned (PR #2027 review on registry.py:141)Kanban: t_7f43f306
Summary by CodeRabbit
New Features
Bug Fixes