fix(security): address 8 CodeRabbit findings — aliasing, permissions, race, snapshot, signing-failure gate, 422 test#2026
fix(security): address 8 CodeRabbit findings — aliasing, permissions, race, snapshot, signing-failure gate, 422 test#2026hognek 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.
…ace, snapshot, signing failures, 422 test 8 CodeRabbit findings fixed: store_signing.py: - Docstring: document detached in-memory signatures (not YAML) + post-load model - _enforce_permissions: fail-closed — raise PermissionError instead of swallowing OSError - Keypair creation race: use os.link for non-overwriting claim instead of os.replace app.py: - Import aliasing: alias store_signing.load_or_create_signing_keypair to prevent shadowing by agent_registry_store version - Move signing key init from create_app() into lifespan (before _startup_complete) registry.py: - _CatalogState: bundle catalog/signatures/manifest_dicts into one atomic snapshot, preventing TOCTOU across the 3 independent assignments - signing_failures: track manifests that failed sign_manifest so install gate can block them (not treat as merely unsigned) store_install.py: - _verify_manifest_for_install: check is_signing_failure() before allowing unsigned manifests through tests: - test_unknown_backend_returns_422: expect 422 (matches production code)
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? |
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ 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}
| # Verify the backend manifest's signature before running its installer. | ||
| # A backend manifest that was tampered with after catalog load would | ||
| # run an untrusted script/image — reject it here. | ||
| be_verified, be_verify_err = _verify_manifest_for_install( |
There was a problem hiding this comment.
WARNING: Backend manifest has no TOCTOU re-verify guard before its installer runs.
The primary manifest gets a disk re-read + signature re-verify at lines 790-829 right before install, closing the window between the gate check (line 764) and execution. The backend manifest here is verified once at the gate (this _verify_manifest_for_install call, which does re-read disk via registry.verify_manifest_signature), but then backend_installer.install(...) runs an untrusted script/image at lines 940-944 with no equivalent re-verify immediately before execution. An attacker who swaps the backend manifest.yaml on disk between this gate check and the actual install (which can be a non-trivial async operation — e.g. model download/resolution) would execute the tampered backend installer, defeating the very protection this PR adds for the primary manifest.
Apply the same disk re-verify guard (re-read the backend manifest from disk and re-check _verify_manifest_for_install / signature) immediately before backend_installer.install(...).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| as 0600, allowing the caller to disable signing rather than read a | ||
| potentially group/world-readable private key. | ||
| """ | ||
| if (keyfile.stat().st_mode & 0o777) != 0o600: |
There was a problem hiding this comment.
SUGGESTION: keyfile.stat() is now unguarded, so any OSError from stat() (e.g. transient EIO, a filesystem that doesn't report permission bits, or a symlink edge case) propagates as a raw OSError, while the failure-mode you intend to surface is PermissionError.
This is fail-closed by design and the lifespan (app.py:1362) does catch (OSError, PermissionError), so signing is only harmlessly disabled — but the raised exception type is misleading and a non-permission OSError is indistinguishable from a true permission violation when logged. Consider wrapping the stat() calls so that non-PermissionError OSErrors are either re-raised as PermissionError deliberately or explicitly allowed to bubble with a clear message.
| if (keyfile.stat().st_mode & 0o777) != 0o600: | |
| try: | |
| mode = keyfile.stat().st_mode & 0o777 | |
| except OSError as exc: | |
| raise PermissionError(f"cannot stat keyfile {keyfile}: {exc}") | |
| if mode != 0o600: | |
| os.chmod(keyfile, 0o600) | |
| if (keyfile.stat().st_mode & 0o777) != 0o600: | |
| raise PermissionError(f"cannot enforce 0600 permissions on {keyfile}") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found (incremental) | Recommendation: Merge OverviewIncremental review of commit Resolved Findings (verified against current HEAD)
Files Reviewed (1 file changed in increment)
Previous Review Summaries (2 snapshots, latest commit 7346261)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7346261)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files changed since last review)
Previously reported findings — status
Previous review (commit dc3cbc2)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by hy3:free · Input: 38.6K · Output: 2.3K · Cached: 93.6K |
… keyfile.stat() OSErrors - Backend manifest TOCTOU: re-read backend manifest.yaml from disk and re-verify signature immediately before backend_installer.install(), matching the primary manifest guard at lines 783-829. - store_signing: wrap keyfile.stat() calls in _enforce_permissions with try/except so non-PermissionError OSErrors are raised as PermissionError instead of raw OSError.
| be_stored_sig = registry.get_signature(result.backend_id) | ||
| if be_stored_sig is not None: | ||
| from tinyagentos.store_signing import verify_manifest_signature as _verify_sig | ||
| if not _verify_sig(on_disk_be, be_stored_sig, _store_pub): |
There was a problem hiding this comment.
[SUGGESTION]: Secondary TOCTOU re-verify is redundant with the primary gate and does not close a real race.
The initial gate at line 924 (_verify_manifest_for_install) already re-reads the backend manifest from disk and verifies it via registry.verify_manifest_signature (see its docstring: "Re-reads the manifest from disk at verify time"). So this second disk re-read + re-verify duplicates the first check's coverage.
More importantly, the window this guard claims to defend ("modified between signature verification and install") is the span from line 924 to line 983, and there is no await/yield anywhere in between (the next await is backend_installer.install(...) at line 983). The coroutine cannot be preempted in that synchronous stretch, so an attacker cannot swap manifest.yaml on disk mid-block. The real TOCTOU surface is between catalog-load and the gate, which the primary gate already re-reads from disk for. This block therefore adds ~40 lines of logic (and two more disk reads) without reducing an actual attack window.
If you want a meaningful post-gate guard, the check should be moved to sit immediately before the await backend_installer.install(...) after any other awaits, or the primary gate's already-on-disk verification is sufficient and this block can be removed. At minimum, consider collapsing this into a single verify call rather than reading get_signature separately and re-reading the file a second time.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…anifest The primary gate at line 924 (_verify_manifest_for_install) already re-reads the manifest from disk and verifies its signature. No await/yield exists between the primary gate and the install call, so the coroutine cannot be preempted in that synchronous stretch. The secondary re-read + re-verify (~40 lines) adds no real attack-window reduction. Reported-by: Kilo Code bot
|
Folded into #2023 |
1 participant
fix(security): address 8 CodeRabbit findings on PR #2023 store security
What changed
store_signing.py (3 fixes)
app.py (2 fixes)
registry.py (2 fixes)
store_install.py (1 fix)
tests (1 fix)
Test results
Fixes: CodeRabbit 7 inline + 1 outside-diff findings on #2023
Related: #1924