fix(store): address 5 security findings from jaylfc review of #1924#2023
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.
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: 38 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 (1)
📝 WalkthroughWalkthroughAdds persistent Ed25519 signing for store manifests, registry-level verification with on-disk rechecks, application key initialization, install-route enforcement, a signing public-key endpoint, and comprehensive signing and route tests. ChangesStore catalog signing and install enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant install_app
participant AppRegistry
participant store_signing
Client->>install_app: Request /api/store/install-v2
install_app->>AppRegistry: Verify catalog manifest
AppRegistry->>store_signing: Verify canonical manifest signature
store_signing-->>AppRegistry: Verification result
AppRegistry-->>install_app: Permit or reject installation
install_app-->>Client: HTTP 200 or HTTP 403
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}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Review (vs
|
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 0 |
| SUGGESTION | 0 |
Incremental Review (vs c15efd7f)
The increment is larger than the prior summary noted — it contains production-code changes (not just test updates) across four files:
tinyagentos/app.py— Keypair load moved fully into the lifespan; catalog-signing loader aliased toload_or_create_store_signing_keypairso it no longer reuses the agent-registry key (resolves the key-reuse finding).create_app()now leavesstore_signing_pubkey = Nonefor callers that bypass the lifespan.tinyagentos/registry.py— Catalog state bundled into a frozen, atomically-replaced_CatalogStatesnapshot (closes the mixed-old/new state TOCTOU).verify_manifest_signatureis now fail-closed for unsigned manifests; signing failures are tracked insigning_failuresand exposed viais_signing_failure().tinyagentos/routes/store_install.py—_verify_manifest_for_installnow blocks manifests that failed to sign during catalog load (treatingsigning_failureas distinct from merely-unsigned). The unknown-backend path returns 422 (test-only assertion tracking was already updated to 422 in both test files).tinyagentos/store_signing.py—_enforce_permissionsnow raisesPermissionErrorwhen 0600 cannot be enforced (fail-closed on loose perms). Concurrent-creation race fixed: shared-tmpunlink()recovery removed in favor of a PID-scoped tmp and a non-overwritingos.linkclaim; a late contender loads the winner's key instead of overwriting it.
All previously-active Kilo findings are resolved in this increment. The corresponding CodeRabbit items (key reuse, signing-failure bypass, chmod fail-closed, immutable snapshot) are also marked addressed. No new issues found in the changed lines.
Files Reviewed (4 files)
tinyagentos/app.py- keypair deferral + loader alias, no new issuestinyagentos/registry.py-_CatalogStatesnapshot + fail-closed verify +is_signing_failure, no new issuestinyagentos/routes/store_install.py- signing-failure gate + 422 backend mapping, no new issuestinyagentos/store_signing.py- permission fail-closed + race fix, no new issues
Previous Findings (all resolved in this increment)
store_signing.pyFileExistsError race — fixed (PID-scoped tmp +os.linknon-overwriting claim).registry.pyfail-openverify_manifest_signature— fixed (now fail-closed; fail-open owned by gate viais_signing_failure).store_install.pyTOCTOU/field-whitelist guard — replaced by signature re-verify + signing-failure block.app.pysigning-key loader aliasing (key reuse) — fixed (load_or_create_store_signing_keypair).registry.py/store_install.pysigning-failure bypass — fixed (is_signing_failureblocks).store_signing.pychmod swallow — fixed (raisesPermissionError).
Previous review (commit c15efd7)
Status: No Issues Found | Recommendation: Merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 0 |
| SUGGESTION | 0 |
Incremental Review (vs ba9e17f9)
The incremental diff contains only test-file changes that update status-code assertions from 500 to 422 in two test files:
tests/routes/test_store_install_v2.pytests/test_routes_store_install.py
These updates correctly track PR change #5 (_BACKEND_TO_METHOD lookup failure now returns 422 instead of 500). Verified the production code at tinyagentos/routes/store_install.py:975 already returns 422 for the unmapped-backend case, so the tests now match actual behavior. No production logic changed in this increment.
Files Reviewed (2 files)
tests/routes/test_store_install_v2.py- test-only assertion update, no issuestests/test_routes_store_install.py- test-only assertion update, no issues
Previous Findings
The prior review (ba9e17f9) reported No Issues Found with both prior findings resolved (registry.py fail-closed, store_install.py TOCTOU disk re-verify). No previously-active Code Review Findings remain open; the 10 legacy inline comments on store_signing.py / registry.py / app.py / store_install.py are outside this increment's changed lines and are not carried forward per incremental scope.
Previous review (commit ba9e17f)
Status: No Issues Found | Recommendation: Merge
Files Reviewed (2 files)
tinyagentos/registry.py- previous SUGGESTION resolved (now fail-closed)tinyagentos/routes/store_install.py- previous SUGGESTION resolved (disk re-verify)
Incremental Review Notes
The incremental diff (vs 60eefc2) addresses both prior findings:
tinyagentos/registry.py:221—verify_manifest_signaturenow returnsFalsefor unsigned manifests (fail-closed), with the fail-open decision moved to_verify_manifest_for_install(short-circuits onget_signature()at store_install.py:230-234). Fix verified.tinyagentos/routes/store_install.py:778-817— The TOCTOU guard now re-reads the manifest from disk and re-verifies the Ed25519 signature instead of a narrow field whitelist, eliminating both the false-positive on catalog reloads and the missing-field gap. Fix verified.
No new issues found in the changed lines. The install-gate fail-open policy remains correct for unsigned manifests.
Previous review (commit 60eefc2)
Status: 2 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 0 |
| SUGGESTION | 2 |
Issue Details (click to expand)
SUGGESTION
| File | Line | Issue |
|---|---|---|
tinyagentos/registry.py |
217 | verify_manifest_signature still fails open (returns True) for unsigned manifests, changing its contract from fail-closed. Footgun for future callers expecting fail-closed; better to keep this primitive fail-closed and let _verify_manifest_for_install own the fail-open decision. |
tinyagentos/routes/store_install.py |
794 | The TOCTOU block is partly redundant with the disk-re-reading signature gate, its field whitelist is incomplete (misses download_url/script/image/env), and can false-positive 403 on legitimate catalog reloads. |
Files Reviewed (3 files)
tinyagentos/store_signing.py- 0 issues (previous WARNING race fixed in this diff)tinyagentos/registry.py- 1 issue (carried forward, still active at HEAD)tinyagentos/routes/store_install.py- 1 issue (carried forward, still active at HEAD)
Fix these issues in Kilo Cloud
Previous review (commit 52fc402)
Status: 3 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 1 |
| SUGGESTION | 2 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
tinyagentos/store_signing.py |
148 | Race in FileExistsError recovery can unlink the winner's in-progress tmp keyfile, causing a winner crash (FileNotFoundError) or an inconsistent on-disk key vs. returned priv. |
SUGGESTION
| File | Line | Issue |
|---|---|---|
tinyagentos/registry.py |
217 | verify_manifest_signature now fails open (returns True) for unsigned manifests — a contract change that is a footgun for future callers expecting fail-closed. |
tinyagentos/routes/store_install.py |
794 | TOCTOU block is largely redundant with the disk-re-reading signature gate, its field whitelist is incomplete (misses download_url/script/image/env), and it can false-positive 403 on legitimate catalog reloads. |
Files Reviewed (6 files)
tinyagentos/store_signing.py- 1 issuetinyagentos/registry.py- 1 issuetinyagentos/routes/store_install.py- 1 issuetinyagentos/app.py- 0 issuestests/test_store_signing.py- 0 issuestests/test_routes_store_install.py- 0 issues
Reviewed by hy3:free · Input: 34K · Output: 1.5K · Cached: 112.8K
…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.
|
Addressing Kilo review (3 issues: 0 CRITICAL, 1 WARNING, 2 SUGGESTIONS): WARNING — store_signing.py:148 (race in FileExistsError recovery): Fixed in 60eefc2. Removed the SUGGESTION — registry.py:217 (fail-open contract change): The fail-open policy is intentional per jaylfc's review (#1924 item 2): the absence of a signature is not evidence of tampering, and rejecting unsigned manifests would block every catalog entry that predates the signing feature. The docstring explicitly documents this policy. SUGGESTION — store_install.py:794 (TOCTOU guard): The TOCTOU guard is defense-in-depth requested by jaylfc (#1924 item 3). While the signature gate re-reads from disk, the TOCTOU guard protects the narrow window between verification and execution — a different threat than the signature check. The field whitelist (id, type, version, install.method, variants count) is intentionally narrow to avoid false positives from YAML formatting or non-functional metadata changes. It will not 403 on legitimate catalog reloads because a reload updates the in-memory manifest to match the on-disk file. |
…_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.
Kilo SUGGESTIONS addressed (ba9e17f)Two fixes pushed: 1. registry.py — verify_manifest_signature now fail-closed (was fail-open) Changed the unsigned-manifest return from 2. store_install.py — replaced field-comparison TOCTOU guard with second signature re-verify The old guard compared 5 whitelisted fields (id, type, version, install.method, variants count) against the re-read disk bytes. This was brittle:
The replacement re-verifies the Ed25519 signature against the re-read bytes. This catches any change, does not false-positive on catalog reloads (which update signatures), and aligns with the existing trust model. Tests: 34/34 pass (registry + store_signing + store_install targeted) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/routes/store_install.py (1)
964-975: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the existing status-code test to expect
422.
tests/test_routes_store_install.pyLines 340-357 still names this casetest_unknown_backend_returns_500and asserts HTTP 500, so this change deterministically breaks that test.Proposed test update
-async def test_unknown_backend_returns_500(self, client): - """A backend not in _BACKEND_TO_METHOD returns 500, not an exception.""" +async def test_unknown_backend_returns_422(self, client): + """A backend not in _BACKEND_TO_METHOD returns 422.""" ... - assert resp.status_code == 500 + assert resp.status_code == 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 `@tinyagentos/routes/store_install.py` around lines 964 - 975, The existing unknown-backend status test must reflect the installer mapping response now returning HTTP 422. Update test_unknown_backend_returns_500 in tests/test_routes_store_install.py to expect 422 and rename it to describe the 422 response, preserving the rest of its assertions.
🤖 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: The store signing keypair initialization must occur
inside lifespan() rather than during create_app(). Leave eager state initialized
to None for callers that bypass lifespan, then move
load_or_create_signing_keypair(), registry.set_signing_key(), warning handling,
and store_signing_pubkey assignment into lifespan() before _startup_complete is
set to True, ensuring registry.reload() is not triggered synchronously during
app creation.
- Line 259: Alias the tinyagentos.store_signing.load_or_create_signing_keypair
import to a distinct store-signing name, and update the catalog-signing call
near agent_registry_store.load_or_create_signing_keypair to use that alias. Keep
the agent-registry loader unchanged so catalog signing uses
store_signing_key.json and its permission handling.
In `@tinyagentos/registry.py`:
- Around line 148-151: The registry currently publishes catalog data, manifest
dictionaries, and signatures independently, allowing verification and
installation to use different reload generations. In tinyagentos/registry.py
lines 148-151, create and publish one immutable, versioned snapshot containing
all three data sets; in tinyagentos/routes/store_install.py lines 751-800,
retrieve the manifest and signature from that same snapshot, verify them
together, and pass the snapshot’s manifest data to the installer.
- Around line 137-145: Ensure signing failures cannot bypass the install gate:
in tinyagentos/registry.py lines 137-145, reject manifests that fail
sign_manifest or cache an explicit signing-error status rather than treating
them as merely unsigned; in tinyagentos/routes/store_install.py lines 230-234,
reject missing signatures whenever signing is configured, permitting them only
when the registry explicitly marks the entry as trusted legacy content.
In `@tinyagentos/store_signing.py`:
- Around line 3-24: Correct the module documentation to describe signatures as
post-load tamper detection: manifests are signed from their contents when
loaded, so pre-load supply-chain or disk tampering is trusted. Document that
signatures are stored as detached in-memory values in AppRegistry._signatures
rather than written to the manifest YAML, and update the verification
description to match this model.
- Around line 135-166: The atomic promotion in the signing-keypair creation flow
can overwrite a keypair created by another process after the shared-temp
timeout. Update the write-and-promote logic around keyfile and tmp so final
publication is a non-overwriting claim, using an appropriate lock or exclusive
link/creation of the completed temporary file; when another process wins,
discard or ignore the local result and load the existing keypair via
load_or_create_signing_keypair. Ensure all contenders return the same persisted
identity.
- Around line 78-107: Update _enforce_permissions to propagate stat/chmod
failures instead of swallowing OSError, and raise when the keyfile permissions
cannot be confirmed as 0600. Ensure load_or_create_signing_keypair stops before
keyfile.read_text when enforcement fails, allowing initialization to disable
signing rather than reading an insufficiently protected private key.
---
Outside diff comments:
In `@tinyagentos/routes/store_install.py`:
- Around line 964-975: The existing unknown-backend status test must reflect the
installer mapping response now returning HTTP 422. Update
test_unknown_backend_returns_500 in tests/test_routes_store_install.py to expect
422 and rename it to describe the 422 response, preserving the rest of its
assertions.
🪄 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: 3ec16caf-418b-4b83-bc92-28d3a996c76b
📒 Files selected for processing (6)
tests/test_routes_store_install.pytests/test_store_signing.pytinyagentos/app.pytinyagentos/registry.pytinyagentos/routes/store_install.pytinyagentos/store_signing.py
|
Nearly there - the folds look right, just two stale tests to update. Your 500->422 change is correct (it addresses my finding that the not-configured/unknown-backend state should be a 4xx, not a server-fault 500), but these two still assert the old 500:
|
Rename test_unknown_backend_returns_500(_not_exception) to _returns_422 in both test_store_install_v2 and test_routes_store_install, matching the new 422 response code jaylfc requested for unknown backends.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_routes_store_install.py (2)
528-529: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the descriptor returned by
mkstemp.
tempfile.mkstemp()returns an open file descriptor; indexing only the path leaks that descriptor during the test run. Close it explicitly or use a context-managed temporary file.Proposed fix
+ import os + - installed_path = Path(tempfile.mkstemp(suffix=".json")[1]) + fd, installed_name = tempfile.mkstemp(suffix=".json") + os.close(fd) + installed_path = Path(installed_name)🤖 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 528 - 529, Update the temporary-file setup around installed_path to capture and explicitly close the file descriptor returned by tempfile.mkstemp before using the path, while preserving the existing valid-JSON initialization.
497-504: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftExercise the second verification, not only the initial gate.
This test tampers with the file between requests, so the second request fails during its initial signature check. It never proves that the manifest is rejected when it changes after that check. Make the real registry’s first verification return successfully, mutate the file as a side effect, then assert that the subsequent on-disk re-verification returns 403.
🤖 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 497 - 504, Update test_real_registry_detects_post_load_tampering so the real registry’s initial manifest verification succeeds, then mutate the manifest as a side effect after that first check and before the install-time re-verification. Keep the assertion focused on the subsequent on-disk verification rejecting the tampered manifest with HTTP 403, rather than failing during the initial signature gate.
🤖 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.
Outside diff comments:
In `@tests/test_routes_store_install.py`:
- Around line 528-529: Update the temporary-file setup around installed_path to
capture and explicitly close the file descriptor returned by tempfile.mkstemp
before using the path, while preserving the existing valid-JSON initialization.
- Around line 497-504: Update test_real_registry_detects_post_load_tampering so
the real registry’s initial manifest verification succeeds, then mutate the
manifest as a side effect after that first check and before the install-time
re-verification. Keep the assertion focused on the subsequent on-disk
verification rejecting the tampered manifest with HTTP 403, rather than failing
during the initial signature gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4f5ae9a-fe84-4a40-ba0e-49b5e415a298
📒 Files selected for processing (2)
tests/routes/test_store_install_v2.pytests/test_routes_store_install.py
|
Heads-up on fragmentation: #2023, #2026, and #2027 all touch the store-signing 500->422 change, and both #2023 and #2026 fail on the SAME stale test (test_unknown_backend_returns_500_not_exception asserts 500, now correctly 422) while #2027 fixes exactly that test in a separate PR. So none of the three can go green on their own - the 500->422 code and its test update have to be in the same PR. Cleanest: fold the #2027 assertion fix into whichever PR carries the code change (this one), rename the two tests to _returns_422, and close the duplicate PRs so we do not merge a half. Then it is a clean merge. The 500->422 direction itself is correct (addresses my #1924 finding that a config/unknown-backend state should be 4xx, not a server-fault 500). |
…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
Consolidation updateFolded #2026 (CodeRabbit fixes) and #2027 (500→422 test assertion) into this PR. Cherry-picked from #2026:
#2027 was already superseded by the existing Both #2026 and #2027 have been closed with 'Folded into #2023'. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/store_signing.py (1)
137-140: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInfinite recursion when
keyfileis corrupt.If the existing keyfile is corrupt (e.g., contains invalid JSON), the function catches the exception and falls through to regenerate the keypair. However, because the corrupt file remains on disk, the atomic promotion via
os.link(tmp, keyfile)will consistently fail withFileExistsError. This triggers the fallback logic to discard the temporary file and recursively callload_or_create_signing_keypair(data_dir), which again reads the corrupt file, leading to infinite recursion.Delete the corrupt file before falling through to regeneration so
os.linkcan succeed.🐛 Proposed fix
except (json.JSONDecodeError, KeyError, ValueError) as exc: logger.warning( "store signing keyfile corrupt (%s), regenerating", exc, ) + keyfile.unlink(missing_ok=True)🤖 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/store_signing.py` around lines 137 - 140, Delete the existing corrupt keyfile in the exception handler before falling through to keypair regeneration. Update the handler around load_or_create_signing_keypair so the subsequent atomic os.link promotion can create the replacement, while preserving the warning and normal regeneration flow.
🧹 Nitpick comments (1)
tinyagentos/store_signing.py (1)
96-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain exceptions in
_enforce_permissions.Within the
exceptclauses, raise thePermissionErrorwithfrom excto preserve the original traceback.♻️ Proposed refactor
try: mode = keyfile.stat().st_mode & 0o777 except OSError as exc: - raise PermissionError(f"cannot stat keyfile {keyfile}: {exc}") + raise PermissionError(f"cannot stat keyfile {keyfile}: {exc}") from exc if mode != 0o600: os.chmod(keyfile, 0o600) try: mode_after = keyfile.stat().st_mode & 0o777 except OSError as exc: - raise PermissionError(f"cannot stat keyfile {keyfile}: {exc}") + raise PermissionError(f"cannot stat keyfile {keyfile}: {exc}") from exc if mode_after != 0o600: raise PermissionError(f"cannot enforce 0600 permissions on {keyfile}")🤖 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/store_signing.py` around lines 96 - 107, Update both OSError handlers in _enforce_permissions to chain the newly raised PermissionError from the caught exception using from exc, preserving the original traceback while keeping the existing error messages and permission checks unchanged.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@tinyagentos/store_signing.py`:
- Around line 137-140: Delete the existing corrupt keyfile in the exception
handler before falling through to keypair regeneration. Update the handler
around load_or_create_signing_keypair so the subsequent atomic os.link promotion
can create the replacement, while preserving the warning and normal regeneration
flow.
---
Nitpick comments:
In `@tinyagentos/store_signing.py`:
- Around line 96-107: Update both OSError handlers in _enforce_permissions to
chain the newly raised PermissionError from the caught exception using from exc,
preserving the original traceback while keeping the existing error messages and
permission checks unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 92eb8019-2426-4fd9-8351-4f90a46e3581
📒 Files selected for processing (4)
tinyagentos/app.pytinyagentos/registry.pytinyagentos/routes/store_install.pytinyagentos/store_signing.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tinyagentos/registry.py
- tinyagentos/routes/store_install.py
- tinyagentos/app.py
…recursion, exception chaining, mkstemp fd leak, TOCTOU test coverage - store_signing.py: unlink corrupt keyfile before regeneration to prevent infinite recursion when os.link fails with FileExistsError on the stale corrupt file - store_signing.py: chain PermissionError with 'from exc' in _enforce_permissions for better traceback preservation - test_routes_store_install.py: close mkstemp file descriptor to prevent fd leak - test_routes_store_install.py: monkeypatch initial signing gate in tampering test so the install-time TOCTOU re-verification is independently exercised All 32 targeted tests pass (store_signing + registry + store_install signing tests)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_routes_store_install.py (1)
497-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp catalog dir/file are never cleaned up.
tempfile.mkdtemp()(line 520) andtempfile.mkstemp()(line 535) create a directory and a file that are never removed after the test. The fd leak is fixed (os.close(fd)), but the underlying directory tree and JSON file are left on disk on every test run. Prefer pytest'stmp_pathfixture, which auto-cleans and removes the manualos/fdbookkeeping entirely.♻️ Proposed refactor using `tmp_path`
- async def test_real_registry_detects_post_load_tampering(self, client): + async def test_real_registry_detects_post_load_tampering(self, client, tmp_path): ... - import os import tempfile from pathlib import Path from tinyagentos.registry import AppRegistry from tinyagentos.store_signing import generate_signing_keypair # 1. Create a catalog directory with one service manifest on disk. - catalog_dir = Path(tempfile.mkdtemp()) + catalog_dir = tmp_path / "catalog" svc_dir = catalog_dir / "services" / "test-svc" svc_dir.mkdir(parents=True) ... priv, pub = generate_signing_keypair() - fd, installed_name = tempfile.mkstemp(suffix=".json") - os.close(fd) - installed_path = Path(installed_name) + installed_path = tmp_path / "installed.json" installed_path.write_text("[]") # initialise with valid JSON🤖 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 497 - 601, Update test_real_registry_detects_post_load_tampering to accept pytest’s tmp_path fixture and create the catalog, manifest, and installed JSON paths beneath it instead of using tempfile.mkdtemp/mkstemp. Remove the tempfile and os imports, manual file-descriptor handling, and explicit temporary-name bookkeeping while preserving the existing test behavior and path layout.
🤖 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.
Nitpick comments:
In `@tests/test_routes_store_install.py`:
- Around line 497-601: Update test_real_registry_detects_post_load_tampering to
accept pytest’s tmp_path fixture and create the catalog, manifest, and installed
JSON paths beneath it instead of using tempfile.mkdtemp/mkstemp. Remove the
tempfile and os imports, manual file-descriptor handling, and explicit
temporary-name bookkeeping while preserving the existing test behavior and path
layout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f55a8949-d164-4970-b154-13838a82b602
📒 Files selected for processing (2)
tests/test_routes_store_install.pytinyagentos/store_signing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tinyagentos/store_signing.py
Replace tempfile.mkdtemp/mkstemp with pytest's tmp_path fixture in test_real_registry_detects_post_load_tampering, which gives automatic teardown and removes manual fd/os/tempfile bookkeeping. Addresses CodeRabbit nitpick (run f55a8949) in PR jaylfc#2023.
…_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.
2 participants
Summary
Fixes 5 security items raised in jaylfc's review of #1924 (code-signing-store-install).
Changes
store_signing.py: umask bypass —
_enforce_permissionsnow runs BEFORE reading the keyfile, so a key written under a loose umask is repaired before the bytes touch process memory.registry.py: fail-open policy —
verify_manifest_signaturenow returnsTrue(fail-open) when no signature is stored for an app, matching the gate's behavior in_verify_manifest_for_install. The fail-open policy is documented in the docstring. Addedset_signing_key()method to support lazy keypair loading.store_install.py: TOCTOU guard — After signature verification passes, the manifest is re-read from disk and critical fields (id, type, version, install.method, variants count) are compared against the in-memory manifest object. If they differ, the install is blocked with 403 — closing the gap between boot-time verification and install-time execution.
app.py: lazy keypair loading — Keypair loading is deferred from
create_app()(eager) to the lifespan, so a read-only data_dir no longer bricks startup. The registry is created withsigning_key=Noneand configured viaregistry.set_signing_key()in the lifespan if the keypair loads successfully.store_install.py: 500→422 — Changed the
_BACKEND_TO_METHODlookup failure from 500 to 422 (Unprocessable Entity). A backend without an installer mapping is a configuration/availability issue, not a server crash.Tests
test_real_registry_detects_post_load_tamperingwhereinstalled_pathwas an empty file causing JSONDecodeErrorTask
Kanban: t_519dc095
PR: #1924
Summary by CodeRabbit
GET /api/store/signing-pubkeyto return the PEM public key (HTTP 404 when unset).