fix(store): rebase onto origin/dev — CI fix for #2026#2039
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)
… 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.
…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
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: 6 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}
| finally: | ||
| os.close(fd) | ||
| try: | ||
| os.link(tmp, keyfile) |
There was a problem hiding this comment.
WARNING: os.link regresses portability vs the previous os.replace.
This commit replaced the atomic os.replace(tmp, keyfile) promotion with os.link(tmp, keyfile) to get non-overwriting semantics. However, os.link only catches FileExistsError here. Hard links are not universally supported: on filesystems/mounts without hardlink support (some overlayfs/container layers, FAT, certain FUSE and network mounts) os.link raises a generic OSError (e.g. EPERM/ENOSYS/EXDEV), not FileExistsError.
Consequences:
- The generic
OSErrorpropagates uncaught out ofload_or_create_signing_keypair(only theFileExistsErrorbranch is handled). It is caught by the lifespan inapp.py, but the net effect is that keypair creation always fails on those filesystems — silently disabling store signing whereos.replacewould have succeeded. - On that error path the
tmpfile is leaked (never unlinked).
Consider keeping os.replace and instead guarding the overwrite race with the existing O_CREAT|O_EXCL claim, or catch the broad OSError around os.link (falling back / cleaning up tmp) so an unsupported-link filesystem does not permanently disable signing and leak temp files.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file in incremental diff)
Notes
Previous Review Summary (commit a82bd88)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a82bd88)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Notes
Reviewed by hy3:free · Input: 33.6K · Output: 1.3K · Cached: 73.7K |
… filesystems Kilo WARNING: os.link raises generic OSError (not FileExistsError) on filesystems without hardlink support (overlayfs, FUSE, container layers, FAT). Catch the broad OSError and fall back to os.replace, which is universally supported but carries a small overwrite-race window bounded by the tmp file's exclusive-open and 0600 permissions. Reported-by: Kilo Code bot
|
Closing: this content already landed. #2023 (the consolidated store-signing PR from the same fix/cr-bot-2023 lineage) was merged to dev earlier today with all 5 findings folded and the 500 to 422 change plus its tests in one PR. The conflicts here are the merged content colliding with itself. If anything in this rebase is genuinely new beyond #2023, open a fresh focused PR with just that delta. |
2 participants
Rebased
fix/cr-bot-2023ontoorigin/dev(was 73 commits behind). All relevant tests pass locally.What changed: Rebase only — no code changes beyond what was already approved. The CI failures were caused by the branch being stale and behind
dev.Test results (local):
test_store_signing.py: 13/13 passtest_registry.py: 17/17 passtest_toctou_atomicity.py: 22/22 passtest_base_store.py: 7/7 passCloses #2026.
Summary by Gitar
tinyagentos/store_signing.pywith Ed25519 key generation, manifest signing, and signature verification logic.appstartup lifespan.test_tampered_manifest_rejected_403andtest_real_registry_detects_post_load_tamperingto ensure unauthorized manifest modifications are blocked.install-v2unknown backend response from 500 to 422 to improve error handling.This will update automatically on new commits.