Conversation
github-actions
Bot
added
scope: channel/cli
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the IronHub integration, enabling users to browse and install WASM tools and skills directly from the IronHub catalog via both the CLI and a new web-based interface. The implementation includes new API handlers for installation, search, and signing key management, alongside a dedicated installer registry and frontend components. Feedback highlights several critical areas for improvement: a potential Denial of Service vulnerability in artifact downloading due to missing size limits, the need for atomic file operations during tool installation to prevent inconsistent states, and logic inconsistencies in name validation across different modules. Additionally, the review identifies issues with error handling during skill registry reloads and misleading error mapping in the extension manager.
|
|
||
| /// Download an artifact from a URL. | ||
| async fn download_artifact(url: &str) -> Result<bytes::Bytes, RegistryError> { | ||
| pub(crate) async fn download_artifact(url: &str) -> Result<bytes::Bytes, RegistryError> { |
There was a problem hiding this comment.
The download_artifact function downloads the entire response body into memory without a size limit. This can be exploited as a Denial of Service (DoS) vector. Consider adding a max_size parameter (using u64 to ensure infallible conversions from usize on all platforms) and using response.bytes_stream() with a limit to prevent unbounded memory consumption.
References
- Prefer using u64 for length prefixes and size limits to ensure that conversions from usize are infallible, avoiding panics and potential DoS vectors.
| fs::write(&target_wasm, wasm_bytes) | ||
| .await | ||
| .map_err(RegistryError::Io)?; | ||
| confirm_written_size(&target_wasm, wasm_bytes.len()).await?; | ||
| fs::write(&target_caps, caps_bytes) | ||
| .await | ||
| .map_err(RegistryError::Io)?; | ||
| confirm_written_size(&target_caps, caps_bytes.len()).await?; |
There was a problem hiding this comment.
The installation of WASM tools is not atomic. If writing the capabilities file fails after the WASM file has been written, the system is left in an inconsistent state. Consider writing to temporary files and using tokio::fs::rename to ensure both files are updated atomically, adhering to async I/O practices. If implementing a cleanup mechanism on failure, ensure any secondary errors are logged as warnings so the primary error is preserved and returned.
References
- In async functions, use asynchronous I/O operations like tokio::fs to avoid blocking the runtime.
- When a primary operation fails and a subsequent cleanup fails, log the secondary failure as a warning and return the original primary error.
| if !(first.is_ascii_lowercase() || first.is_ascii_digit()) { | ||
| return false; | ||
| } | ||
| chars.all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-') |
There was a problem hiding this comment.
There is an inconsistency between this validation and validate_hub_artifact_name in src/registry/hub_installer.rs. This function only allows hyphens, while the registry validation also allows underscores. Aligning these prevents unexpected rejections. Additionally, validation at this public API boundary must strictly check for path traversal characters (/, , .., \0) to prevent security vulnerabilities.
References
- Path traversal characters in extension names must be validated at the public API boundary for all operations to prevent security vulnerabilities.
| ); | ||
| poison.into_inner() | ||
| }); | ||
| handle.block_on(guard.reload()); |
There was a problem hiding this comment.
The result of guard.reload() is ignored. If the skill registry fails to reload after a new skill is installed, the tool will still report success, but the new skill won't be available to the agent. The return value of block_on should be captured and handled.
| .extension_manager | ||
| .list(Some(ExtensionKind::WasmTool), false, &ctx.user_id) | ||
| .await | ||
| .map_err(|_| catalog_unavailable())? |
There was a problem hiding this comment.
The error mapping catalog_unavailable() is misleading here. extension_manager.list is a local operation on the installed extensions; its failure indicates an internal system error or a problem with the local storage, not an issue with the remote IronHub catalog.
| if store | ||
| .exists(&user.user_id, IRONHUB_SIGNING_KEY_NAME) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? | ||
| { | ||
| store | ||
| .delete(&user.user_id, IRONHUB_SIGNING_KEY_NAME) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; | ||
| } |
There was a problem hiding this comment.
The signing key regeneration process is not atomic. If the delete operation succeeds but the subsequent create operation fails, the user will be left without any signing key. While not critical, it forces the user to manually retry. Consider if the SecretsStore supports an atomic replacement or ensure the old key is preserved until the new one is successfully created.
|
Thanks for the review. Addressed in
On the fmt and clippy (all features) are clean and the targeted tests pass. |
|
Hi @neo-sky, I'm testing the Ironclaw skill and I've noticed that the command shown on the page is different from the PR. Also, this PR doesn't install the Fly skills as is And then there's this type of command that I didn't see implemented: We should standardize this, and I can help you do it, but I'm not sure what the best approach is to solve this. |
|
Thanks Matias, good catches. Quick rundown on each. The command mismatch is the actual install blocker. The CLI is currently Same story with The Fly URL is a separate issue. The catalog manifest the CLI uses tokenizes through hub.ironclaw.com correctly, but the data path the marketplace UI uses (the public-skills endpoint) passes Iliad URLs straight through. I'll tokenize that path too so the marketplace UI shows clean hub.ironclaw.com URLs everywhere and nothing raw leaks out. All three sit on the IronHub side, not in this PR's diff. I'll get the cleanup in and ping you when there's something fresh to retest. |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review
Reviewed PR #3737 at 9d6ca674163663b2522ed5a0d529792c9b84d9a5 with the multi-agent code-review skill. The diff was larger than the reviewer bundle cap, so reviewers used the detached worktree for full file context and line verification.
Outcome: REQUEST_CHANGES
Findings kept after aggregation: 13
- Security/supply-chain: 5
- Bugs/behavior: 3
- Performance/concurrency: 2
- Tests: 5
- Conventions/docs: 1
Highest-priority issues are the manifest re-fetch after the community-content gate, the CLI install path bypassing that gate, ineffective rate limiting on gateway-dispatched IronHub fetches, and missing caller-facing tests for the new signing-key/deep-link/agent-tool paths.
Reviewers failed: none.
|
|
||
| match kind { | ||
| HubEntryKind::Tool => { | ||
| let outcome = installer |
There was a problem hiding this comment.
The install gate validates one live manifest, but this call fetches the manifest again inside install_tool_by_name / install_skill_by_name. Since the default catalog is now live and mutable, the artifact/provenance installed can differ from the entry that passed acknowledge_unverified, which can bypass the community-content gate. Install from the already-vetted manifest with install_tool_from_manifest / install_skill_from_manifest instead.
| match kind { | ||
| Kind::Tool => { | ||
| println!("Installing tool '{}' from IronHub...", name); | ||
| let outcome = installer |
There was a problem hiding this comment.
The CLI install path classifies the entry and immediately installs it without checking provenance.is_community_unverified() or requiring an explicit acknowledgement flag. That leaves ironclaw ironhub install less protected than the agent-callable tool for the same remote supply-chain operation. Please require a dedicated acknowledgement for community/unverified entries and install from the same checked manifest.
| if let Some(tag) = req.release_tag { | ||
| params.insert("release_tag".into(), serde_json::Value::String(tag)); | ||
| } | ||
| params.insert("force".into(), serde_json::Value::Bool(req.force)); |
There was a problem hiding this comment.
The gateway request never forwards acknowledge_unverified, and the DTO rejects unknown fields, so web/deep-link installs of community entries cannot complete even after the user explicitly accepts the risk. Add acknowledge_unverified to the request/UI path and forward it into ironhub_install.
| "properties": { | ||
| "name": { | ||
| "type": "string", | ||
| "pattern": "^[a-z0-9][a-z0-9-]*$", |
There was a problem hiding this comment.
This schema rejects underscores, but validate_hub_name and installer tests accept names like microsoft_365. Because dispatcher schema validation runs before execute, valid catalog entries can work through CLI/installer paths but fail through agent and gateway dispatch. Align the schema with the shared validator or tighten the shared validator to match.
| serde_json::Value::Object(obj) | ||
| } | ||
|
|
||
| fn tool_entry_json(entry: &HubToolEntry) -> serde_json::Value { |
There was a problem hiding this comment.
Search/list JSON rows omit provenance even though the manifest carries it and install output surfaces it. Agent and gateway catalog browsing can therefore present community entries without the required trust/community label until a later info/install step. Include provenance and an unverified/trust label in these row helpers, and mirror the label in CLI list/search output.
| return; | ||
| } | ||
|
|
||
| apiFetch('/api/ironhub/verify-intent', { |
There was a problem hiding this comment.
The new deep-link install route has no browser/e2e coverage for hash parsing, verify-intent, info fetch, and confirm POST behavior. Handler tests will not catch regressions in this user-visible flow. Add an e2e/browser test that drives #/install/<name>?ts=<ts>&sig=<sig>, including invalid links and successful confirm.
| } | ||
| .unwrap_or_default(); | ||
|
|
||
| if provenance.is_community_unverified() && !acknowledge_unverified { |
There was a problem hiding this comment.
The actual ironhub_install execution path has no test proving community content is rejected unless acknowledge_unverified=true. Existing coverage exercises helper output/schema behavior, but not the side-effecting tool call that gates installation. Add a tool execution test for a Provenance::New entry with both false and true acknowledgement values.
| }) | ||
| } | ||
|
|
||
| async fn execute( |
There was a problem hiding this comment.
ironhub_remove is a new side-effecting agent-callable tool, but no adjacent test calls its execute path. Please add coverage for a successful remove and for the still-present verification failure so name validation, extension-manager error mapping, and post-remove verification cannot regress silently.
| tracing::debug!("Registered 4 skill management tools"); | ||
| } | ||
|
|
||
| pub fn register_ironhub_tools(&self, deps: crate::tools::builtin::IronhubDeps) { |
There was a problem hiding this comment.
This registration function is the composition point that makes the five IronHub built-ins available, but registry tests do not assert the resulting tool names. Add a registry test that calls register_ironhub_tools and verifies ironhub_install, ironhub_remove, ironhub_search, ironhub_list, and ironhub_info are registered.
| axum::routing::delete(skills_remove_handler), | ||
| ) | ||
| // IronHub catalog | ||
| .route("/api/ironhub/install", post(ironhub_install_handler)) |
There was a problem hiding this comment.
This adds a new /api/ironhub/* route family, but src/channels/web/CLAUDE.md still has no IronHub handler entry or route table coverage. The repo rules require behavior changes to update the relevant subsystem docs/specs in the same branch. Please document the handler file and install/search/list/info/signing-key/verify-intent routes there.
Adds the IronHub install pipeline end-to-end: registry-driven install of WASM tools and skills by name with provenance gating and TOCTOU-safe writes, agent-callable install/search/list/info/remove tools, gateway routes for the same surface plus a Settings UI for the shared signing key, HMAC-verified deep-link install flow, and a CLI namespace under `ironclaw hub`. Closes the Firat review cluster (caller-level Provenance::New test, ironhub_remove still-present failure, ack flow forwarded through gateway and UI, schema regex aligned with the shared validator, signing-key handler tests, deep-link Playwright e2e, web/CLAUDE.md docs).
9d6ca67 to
463c73e
Compare
github-actions
Bot
added
scope: agent
github-actions
Bot
added
scope: docs
Thank you very much. I have one last question: how do you recommend I upload updates to the skills on IronHub, such as this one: Should I create an issue and a pull request on IronHub? Or how else could I do it? |
Skill installs now read back the registry after reload and report whether the skill actually loaded, instead of reloading blind. Checksum comparison is case-insensitive so a hash that differs only in casing no longer fails every install, and release tags containing '..' are rejected outright. Adds regression tests for each.
|
@serrrfirat retargeted this to |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review
Reviewers: security, bugs, performance/concurrency, tests, conventions
Verdict: ❌ Request Changes
Stats
| Severity | Count |
|---|---|
| High | 7 |
| Medium | 8 |
| Reviewer | Findings |
|---|---|
| tests | 5 |
| performance | 5 |
| security | 3 |
| bugs | 2 |
Security
[High] Deep-link verification is decoupled from install — admin can install without HMAC proof (confidence: 85%)
File: src/channels/web/handlers/ironhub.rs:117-145
The verify-intent endpoint validates the HMAC signature and records a nonce, then returns {valid:true}. The browser subsequently calls /api/ironhub/install in a completely separate, unlinked request. The install handler only requires AdminUser auth and dispatches directly to the ironhub_install tool — it never re-checks a verification token or proves the caller went through verify-intent. Any admin session can POST /api/ironhub/install with arbitrary names and acknowledge_unverified=true, bypassing the deep-link HMAC gate entirely. The two-step flow (verify then install) provides UX but no security binding.
Fix: Return a short-lived, single-use token from verify-intent; require it in the install request and validate server-side before dispatching.
[Medium] Signing key set handler leaks old key plaintext during error-path restore (confidence: 72%)
File: src/channels/web/handlers/ironhub.rs:204-217
In ironhub_signing_key_set_handler, after deleting the old key and failing to create the new one, the handler attempts to restore the old key. The old key was retrieved as plaintext via get_decrypted().expose().to_string() and stored in the previous variable. If both create and restore fail, only a tracing::warn is emitted. More critically, the old key material lives as a String on the stack longer than necessary and could appear in core dumps.
Fix: Use secrecy::SecretString for the previous key material and zeroize on drop. Minimize the window where the old key is in cleartext.
Also flagged by: tests
[Medium] HMAC uses raw shared-key UTF-8 bytes as MAC key — low-entropy keys accepted (confidence: 70%)
File: src/channels/web/handlers/ironhub.rs:241-248
hmac_hex uses shared_key.as_bytes() directly as the HMAC key. The validate_shared_key function only requires a minimum length of 32 characters and the ihub_sk_ prefix. A key like ihub_sk_aaaaaaaaaaaaaaaaaaaaaaaaa (8 prefix + 24 'a's = 32 chars) has extremely low entropy. HMAC-SHA256's security relies on key entropy; with a predictable key, an attacker can forge install signatures.
Fix: Enforce minimum entropy (e.g., reject keys that are all the same character, or require at least N distinct characters), or derive the HMAC key from the shared key via HKDF to normalize entropy.
Bugs
[High] Signing key set/get/delete allows any authenticated user (confidence: 75%)
File: src/channels/web/handlers/ironhub.rs:266-269
ironhub_signing_key_set_handler, _get_handler, and _delete_handler accept AuthenticatedUser while ironhub_install_handler requires AdminUser. Any non-admin authenticated user can set/replace/revoke the signing key that controls deep-link install verification, potentially disabling HMAC verification for an admin's install flow by deleting or replacing the key. The signing key is per-user so one user cannot directly affect another's key, but a compromised regular account can manipulate its own signing key to forge valid deep-link installs.
Fix: Change AuthenticatedUser to AdminUser for all three signing-key handlers, or document that per-user keys are intentional.
Also flagged by: performance
[Medium] install_tool_entry downloads before checking AlreadyInstalled (confidence: 72%)
File: src/registry/hub_installer.rs:193-199
install_tool_entry downloads both wasm and capabilities artifacts before calling install_tool_from_bytes, which only then acquires the lock and checks if target_wasm.exists(). For force=false installs of already-installed tools, this wastes network bandwidth and time downloading artifacts that will never be written. The same issue exists in install_skill_entry.
Fix: Move the already-installed check (or at least an early-exit probe) before download_artifact calls in install_tool_entry and install_skill_entry.
Performance
[High] INSTALL_LOCKS HashMap grows without bound — Arc entries never removed (confidence: 90%)
File: src/registry/hub_installer.rs:17-27
acquire_install_lock inserts an Arc<AsyncMutex<()>> per tool/skill name but never removes entries. A long-lived process installing many distinct names (or a tool name generator) causes unbounded HashMap growth, leaking the mutex + key string forever.
Fix: Add cleanup after install completes (drop + remove if Arc refcount==1), or use an LRU/weak-ref map.
Also flagged by: security, performance
[High] Nonce cache prune+sort under global Mutex blocks all verify-intent requests (confidence: 85%)
File: src/channels/web/handlers/ironhub.rs:22-46
nonce_seen_or_record holds a global std::sync::Mutex while doing O(n) retain, O(n log n) sort, and O(n/4) removes when over capacity. Under load, every verify-intent and register call serializes on this lock, and the eviction path (sort+drain) can take milliseconds with 16K entries, stalling all concurrent HMAC verifications.
Fix: Replace with DashMap or RwLock so reads/writes to different keys proceed concurrently; move eviction to a background task or use an LRU crate with O(1) eviction.
Also flagged by: tests, security, performance, conventions, performance, conventions, security, bugs
[Medium] Every catalog tool call fetches manifest from remote — no caching (confidence: 80%)
File: src/tools/builtin/ironhub.rs:254-260
IronhubInstallTool::execute, IronhubSearchTool, IronhubListTool, and IronhubInfoTool all call fetch_manifest() on every invocation. Under sustained agent-driven catalog queries (e.g. LLM iterating search→info→install), this produces one HTTPS GET per tool call with no TTL or in-memory cache. Multiple concurrent agent turns amplify this.
Fix: Add a time-based cached manifest (e.g. Arc<RwLock<Option<(Instant, HubManifest)>>>) with a TTL of ~60s shared across all four tools.
[Medium] Skill registry reload spawns blocking task holding RwLock write via unwrap_or_else (confidence: 80%)
File: src/tools/builtin/ironhub.rs:284-296
In IronhubInstallTool::install_from_manifest (skill branch), a tokio::task::spawn_blocking call acquires an std::sync::RwLock write guard and then calls handle.block_on(guard.reload()). If reload() is slow or panics, the std::sync::RwLock is poisoned for all future skill registry access. Also, handle.block_on inside spawn_blocking can deadlock if the runtime is saturated.
Fix: Use tokio::sync::RwLock instead of std::sync::RwLock for the skill registry so reload can be awaited directly without spawn_blocking + block_on.
[Medium] AlreadyInstalled check via exists() then write is TOCTOU within the async mutex (confidence: 75%)
File: src/registry/hub_installer.rs:208-215
target_wasm.exists() is checked under the per-name async mutex, but verify_sha256 + write happen after the check. If two install calls for the same name arrive and the first fails after writing the wasm (due to caps write failure), the second may also see AlreadyInstalled from a partial/leftover file. The lock serializes, but the exists() check only guards the happy path.
Fix: Use a rename-from-temp pattern exclusively; check existence of the temp target rather than the final path, or track install state in the lock guard.
Tests
[High] verify-intent returns valid=false when no signing key configured (confidence: 90%)
File: src/channels/web/handlers/ironhub.rs:394-400
ironhub_verify_intent_handler returns {valid: false, reason: 'no signing key configured'} when the user has no signing key. Only the signing-key GET 404 path is tested, but the verify-intent-specific 'no signing key' soft-failure path (returns 200 with valid=false instead of an HTTP error) has no test. This is a distinct security-sensitive path that could regress to a 500 or leak info.
Fix: tests::ironhub::verify_intent_returns_invalid_when_no_signing_key
Also flagged by: conventions, tests, tests
[High] register returns 503 when no signing key configured (confidence: 90%)
File: src/channels/web/handlers/ironhub.rs:458-464
ironhub_register_handler returns 503 SERVICE_UNAVAILABLE with 'no signing key configured on this agent' when the user lacks a signing key. This is distinct from the verify-intent soft-failure and has no test. A regression here would break the IronHub registration handshake silently.
Fix: tests::ironhub::register_rejects_when_no_signing_key_configured
[High] register rejects replayed nonce with 409 CONFLICT (confidence: 88%)
File: src/channels/web/handlers/ironhub.rs:475-482
nonce_seen_or_record is called in ironhub_register_handler and returns 409 CONFLICT on nonce replay. The verify-intent handler's nonce replay is tested, but the register handler's nonce replay path is not. Both handlers share the same global NONCE_CACHE but use different payload formats, so register's nonce rejection is independent and untested.
Fix: tests::ironhub::register_rejects_replayed_nonce_with_409
[Medium] signing-key-set replace-with-existing-key path untested (confidence: 78%)
File: src/channels/web/handlers/ironhub.rs:286-322
ironhub_signing_key_set_handler reads and deletes the existing key before creating the new one. When create fails, it attempts to restore the old key. Neither the success path (replace existing) nor the failure-restore path is tested. The existing tests only cover setting a key when none exists. A regression in the replace logic could silently lose the user's signing key.
Fix: tests::ironhub::signing_key_set_replaces_existing_key
Also flagged by: conventions, bugs, security
[Medium] tool_error_to_http untested branches: Timeout, RateLimited, Sandbox (confidence: 75%)
File: src/channels/web/handlers/ironhub.rs:70-88
tool_error_to_http maps ToolError::Timeout -> 504, RateLimited -> 429, Sandbox -> 500, ExternalService -> 502. Only InvalidParameters and ExecutionFailed are exercised indirectly via the dispatch stub. The Timeout, RateLimited, Sandbox, and ExternalService branches have no unit test. A typo in the match (e.g. swapped status codes) would go undetected.
Fix: tests::ironhub::tool_error_to_http_maps_each_variant
| .get_decrypted(&user.user_id, IRONHUB_SIGNING_KEY_NAME) | ||
| .await | ||
| { | ||
| Ok(s) => s, |
There was a problem hiding this comment.
[High] verify-intent returns valid=false when no signing key configured (confidence: 90%, reviewer: tests)
ironhub_verify_intent_handler returns {valid: false, reason: 'no signing key configured'} when the user has no signing key. Only the signing-key GET 404 path is tested, but the verify-intent-specific 'no signing key' soft-failure path (returns 200 with valid=false instead of an HTTP error) has no test. This is a distinct security-sensitive path that could regress to a 500 or leak info.
Fix: tests::ironhub::verify_intent_returns_invalid_when_no_signing_key
#[tokio::test]
async fn verify_intent_returns_invalid_when_no_signing_key() {
let state = TestGatewayBuilder::new()
.user_id("test-user")
.build();
let app = Router::new()
.route("/api/ironhub/verify-intent", post(ironhub_verify_intent_handler))
.with_state(state);
let body = serde_json::json!({"slug":"x","version":"1","uid":"u","aid":"a","ts":now_unix(),"nonce":"n","sig":"00".repeat(32)});
let req = verify_req(body);
let resp = oneshot(app, req).await;
assert_eq!(resp.status(), StatusCode::OK);
let json = body_json(resp).await;
assert_eq!(json["valid"], false);
assert!(json["reason"].as_str().unwrap().contains("no signing key"));
}Also flagged by: conventions, tests, tests
| let decrypted = match store | ||
| .get_decrypted(&user.user_id, IRONHUB_SIGNING_KEY_NAME) | ||
| .await | ||
| { |
There was a problem hiding this comment.
[High] register returns 503 when no signing key configured (confidence: 90%, reviewer: tests)
ironhub_register_handler returns 503 SERVICE_UNAVAILABLE with 'no signing key configured on this agent' when the user lacks a signing key. This is distinct from the verify-intent soft-failure and has no test. A regression here would break the IronHub registration handshake silently.
Fix: tests::ironhub::register_rejects_when_no_signing_key_configured
#[tokio::test]
async fn register_rejects_when_no_signing_key_configured() {
let state = TestGatewayBuilder::new().user_id("test-user").build();
let app = Router::new()
.route("/api/ironhub/register", post(ironhub_register_handler))
.with_state(state);
let body = serde_json::json!({"uid":"u","aid":"a","ts":now_unix(),"nonce":"n","sig":"00".repeat(32)});
let resp = oneshot(app, register_req(body)).await;
assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE);
}|
|
||
| const MAX_MANIFEST_BYTES: usize = 1024 * 1024; | ||
| const MAX_METADATA_BYTES: usize = 1024 * 1024; | ||
| const MAX_WASM_BYTES: usize = 16 * 1024 * 1024; |
There was a problem hiding this comment.
[High] INSTALL_LOCKS HashMap grows without bound — Arc entries never removed (confidence: 90%, reviewer: performance)
acquire_install_lock inserts an Arc<AsyncMutex<()>> per tool/skill name but never removes entries. A long-lived process installing many distinct names (or a tool name generator) causes unbounded HashMap growth, leaking the mutex + key string forever.
Fix: Add cleanup after install completes (drop + remove if Arc refcount==1), or use an LRU/weak-ref map.
let lock = acquire_install_lock(&key);
let guard = lock.lock().await;
let result = { ... };
let mut map = INSTALL_LOCKS.lock().unwrap();
if let Some(existing) = map.get(&key) {
if Arc::strong_count(existing) == 1 { map.remove(&key); }
}
resultAlso flagged by: security, performance
|
|
||
| use subtle::ConstantTimeEq; | ||
| let supplied = req.sig.as_bytes(); | ||
| let sig_valid: bool = expected.as_bytes().ct_eq(supplied).into(); |
There was a problem hiding this comment.
[High] register rejects replayed nonce with 409 CONFLICT (confidence: 88%, reviewer: tests)
nonce_seen_or_record is called in ironhub_register_handler and returns 409 CONFLICT on nonce replay. The verify-intent handler's nonce replay is tested, but the register handler's nonce replay path is not. Both handlers share the same global NONCE_CACHE but use different payload formats, so register's nonce rejection is independent and untested.
Fix: tests::ironhub::register_rejects_replayed_nonce_with_409
#[tokio::test]
async fn register_rejects_replayed_nonce_with_409() {
let (app, _, _, ts) = verify_app().await;
let nonce = uuid::Uuid::new_v4().to_string();
let payload = register_payload("u1", "a1", ts, &nonce);
let sig = hmac_hex(TEST_SHARED_KEY, &payload).unwrap();
let body = serde_json::json!({"uid":"u1","aid":"a1","ts":ts,"nonce":nonce,"sig":sig});
let first = oneshot(app.clone(), register_req(body.clone())).await;
assert_eq!(first.status(), StatusCode::OK);
let replay = oneshot(app, register_req(body)).await;
assert_eq!(replay.status(), StatusCode::CONFLICT);
}| State(state): State<Arc<GatewayState>>, | ||
| AdminUser(user): AdminUser, | ||
| Json(req): Json<IronhubInstallRequest>, | ||
| ) -> Result<Json<serde_json::Value>, (StatusCode, String)> { |
There was a problem hiding this comment.
[High] Deep-link verification is decoupled from install — admin can install without HMAC proof (confidence: 85%, reviewer: security)
The verify-intent endpoint validates the HMAC signature and records a nonce, then returns {valid:true}. The browser subsequently calls /api/ironhub/install in a completely separate, unlinked request. The install handler only requires AdminUser auth and dispatches directly to the ironhub_install tool — it never re-checks a verification token or proves the caller went through verify-intent. Any admin session can POST /api/ironhub/install with arbitrary names and acknowledge_unverified=true, bypassing the deep-link HMAC gate entirely. The two-step flow (verify then install) provides UX but no security binding.
Fix: Return a short-lived, single-use token from verify-intent; require it in the install request and validate server-side before dispatching.
// verify-intent returns: { valid: true, install_token: "<random>" }
// install handler checks: token is valid, not used, and matches the slug
// Single-use token stored in NONCE_CACHE or a separate map| Ok(()) | ||
| } | ||
|
|
||
| fn tool_error_to_http(err: ToolError) -> (StatusCode, String) { |
There was a problem hiding this comment.
[Medium] tool_error_to_http untested branches: Timeout, RateLimited, Sandbox (confidence: 75%, reviewer: tests)
tool_error_to_http maps ToolError::Timeout -> 504, RateLimited -> 429, Sandbox -> 500, ExternalService -> 502. Only InvalidParameters and ExecutionFailed are exercised indirectly via the dispatch stub. The Timeout, RateLimited, Sandbox, and ExternalService branches have no unit test. A typo in the match (e.g. swapped status codes) would go undetected.
Fix: tests::ironhub::tool_error_to_http_maps_each_variant
#[test]
fn tool_error_to_http_maps_each_variant() {
assert_eq!(tool_error_to_http(ToolError::Timeout(Duration::from_secs(5))).0, StatusCode::GATEWAY_TIMEOUT);
assert_eq!(tool_error_to_http(ToolError::RateLimited(None)).0, StatusCode::TOO_MANY_REQUESTS);
assert_eq!(tool_error_to_http(ToolError::ExternalService("x".into())).0, StatusCode::BAD_GATEWAY);
assert_eq!(tool_error_to_http(ToolError::Sandbox("x".into())).0, StatusCode::INTERNAL_SERVER_ERROR);
assert_eq!(tool_error_to_http(ToolError::NotAuthorized("x".into())).0, StatusCode::UNAUTHORIZED);
}| force: bool, | ||
| ) -> Result<HubInstallOutcome, RegistryError> { | ||
| validate_skill_entry(entry)?; | ||
|
|
There was a problem hiding this comment.
[Medium] AlreadyInstalled check via exists() then write is TOCTOU within the async mutex (confidence: 75%, reviewer: performance)
target_wasm.exists() is checked under the per-name async mutex, but verify_sha256 + write happen after the check. If two install calls for the same name arrive and the first fails after writing the wasm (due to caps write failure), the second may also see AlreadyInstalled from a partial/leftover file. The lock serializes, but the exists() check only guards the happy path.
Fix: Use a rename-from-temp pattern exclusively; check existence of the temp target rather than the final path, or track install state in the lock guard.
| params.insert("name".into(), serde_json::Value::String(q.name)); | ||
| if let Some(tag) = q.release_tag { | ||
| params.insert("release_tag".into(), serde_json::Value::String(tag)); | ||
| } |
There was a problem hiding this comment.
[Medium] Signing key set handler leaks old key plaintext during error-path restore (confidence: 72%, reviewer: security)
In ironhub_signing_key_set_handler, after deleting the old key and failing to create the new one, the handler attempts to restore the old key. The old key was retrieved as plaintext via get_decrypted().expose().to_string() and stored in the previous variable. If both create and restore fail, only a tracing::warn is emitted. More critically, the old key material lives as a String on the stack longer than necessary and could appear in core dumps.
Fix: Use secrecy::SecretString for the previous key material and zeroize on drop. Minimize the window where the old key is in cleartext.
Also flagged by: tests
| ) -> Result<HubInstallOutcome, RegistryError> { | ||
| validate_tool_entry(entry)?; | ||
|
|
||
| let wasm_bytes = download_artifact(&entry.wasm.url, MAX_WASM_BYTES as u64).await?; |
There was a problem hiding this comment.
[Medium] install_tool_entry downloads before checking AlreadyInstalled (confidence: 72%, reviewer: bugs)
install_tool_entry downloads both wasm and capabilities artifacts before calling install_tool_from_bytes, which only then acquires the lock and checks if target_wasm.exists(). For force=false installs of already-installed tools, this wastes network bandwidth and time downloading artifacts that will never be written. The same issue exists in install_skill_entry.
Fix: Move the already-installed check (or at least an early-exit probe) before download_artifact calls in install_tool_entry and install_skill_entry.
pub async fn install_tool_entry(&self, entry: &HubToolEntry, release_tag: &str, force: bool) -> Result<HubInstallOutcome, RegistryError> {
validate_tool_entry(entry)?;
let target_wasm = self.tools_dir.join(format!("{}.wasm", entry.name));
if target_wasm.exists() && !force {
return Err(RegistryError::AlreadyInstalled { name: entry.name.clone(), path: target_wasm });
}
let wasm_bytes = download_artifact(&entry.wasm.url, MAX_WASM_BYTES as u64).await?;
let caps_bytes = download_artifact(&entry.capabilities.url, MAX_METADATA_BYTES as u64).await?;
self.install_tool_from_bytes(entry, release_tag, &wasm_bytes, &caps_bytes, force).await
}| .unwrap_or(0) | ||
| } | ||
|
|
||
| fn hmac_hex(shared_key: &str, msg: &str) -> Result<String, String> { |
There was a problem hiding this comment.
[Medium] HMAC uses raw shared-key UTF-8 bytes as MAC key — low-entropy keys accepted (confidence: 70%, reviewer: security)
hmac_hex uses shared_key.as_bytes() directly as the HMAC key. The validate_shared_key function only requires a minimum length of 32 characters and the ihub_sk_ prefix. A key like ihub_sk_aaaaaaaaaaaaaaaaaaaaaaaaa (8 prefix + 24 'a's = 32 chars) has extremely low entropy. HMAC-SHA256's security relies on key entropy; with a predictable key, an attacker can forge install signatures.
Fix: Enforce minimum entropy (e.g., reject keys that are all the same character, or require at least N distinct characters), or derive the HMAC key from the shared key via HKDF to normalize entropy.
Re-verify the signed deep-link HMAC and consume its nonce at the install boundary, gate signing-key set/get/delete behind admin, replace delete-then-restore with an atomic key upsert, and generate shared keys server-side with an entropy floor. Bound the install lock map, move the nonce cache to a FIFO VecDeque, add a TTL manifest cache, isolate skill reloads on a dedicated runtime, and make installs atomic with write-temp plus rename. Adds regression tests covering the install, nonce, key, admin, manifest-cache, remove, registry, and CLI paths.
github-actions
Bot
added
risk: medium
The gateway now forwards the signed version into ironhub_install, and install_from_manifest refuses to install unless the current catalog entry matches it, so a catalog roll inside the freshness window cannot swap the artifact out from under a signed intent. ironhub_info now emits trust_label via shared info_tool_json/info_skill_json helpers so the deep-link confirm renders the same trust tier as search and list. Adds regression tests for version forward, match, mismatch, and the info trust_label output.
The signed deep-link now carries an artifact_digest, so an install is tied to the exact artifact, not just the version. The agent folds it into the HMAC, recomputes it from the manifest before installing, and bails on mismatch, which catches a same-version content swap. Rust and TS share fixed test vectors so the hash can't drift.
IronHub calls it server-to-server, so the session gate rejected every handshake.
Was a one-off card, now a normal settings row.
The consent-card buttons and the signing-key form used inline onclick/onsubmit handlers that the gateway CSP silently blocks, so Save and the install and cancel buttons did nothing. Moved them to addEventListener bound after render, and fixed the install card rendering bottom-left and leaking across tab switches where an #tab-install rule was overriding the .tab-panel display via ID specificity.
Adds IRONHUB_MANIFEST_URL and IRONHUB_EXTRA_ARTIFACT_HOSTS overrides so the agent can install against staging or a partner catalog without recompiling, both falling back to the production defaults and still validated against the host allowlist. Also logs the underlying RegistryError before the generic catalog-unavailable mapping so operators can tell a 404 from a timeout. Tests cover URL resolution, host parsing, and allowlist-with-extras.
Provenance defaults to New (unverified) instead of Official, so an unknown or absent tier warns instead of installing silently. The artifact-host check now rejects IP literals, loopback, internal DNS, and bare names for every host, including the env-supplied extra hosts.
The agent now requires an Ed25519-signed manifest envelope from the catalog and verifies it against an embedded public key, fail-closed: an unsigned or tampered manifest is rejected before any artifact is fetched, and the private signing key lives only on the IronHub server. --release-tag is disabled because pinning to a GitHub release fetches an unsigned manifest; pinned installs return a clear error pointing at the default signed catalog.
|
@serrrfirat ready for another look when you get a chance, and thanks again for taking the time on this one. The main change since your May 22 review: the catalog manifest is now signed. IronHub returns an Ed25519 envelope and the agent verifies it against an embedded public key before using it, so an unsigned or tampered manifest is rejected before any artifact is fetched. The private key stays on the IronHub server and never lands in this repo or the binary, and a shared test vector keeps the Node signer and the Rust verifier checked against the same bytes. One consequence: Also tightened since you last looked: provenance defaults to the least-trusted tier when the field is missing, the artifact-host check rejects IP literals, localhost, and internal or bare hostnames (HTTPS-only, closed allowlist), signed installs are bound to version and artifact digest with a one-shot nonce, and Validation: fmt and the full clippy gate are clean, and One flag before merge: verification is fail-closed and the default catalog is prod (hub.ironclaw.com), which isn't signing yet, only staging is. We'll deploy prod with the signing key before this ships so agents on the default URL don't reject the unsigned prod manifest. |
…ationale MANIFEST_VERIFY_KEYS now has the rotation procedure inline above the slice (add new key, deploy signer, drop old key in a follow-up release). The skill-registry reload in ironhub_install carries a short comment explaining why it stays on std::sync::RwLock and uses spawn_blocking plus a fresh current-thread runtime plus poison recovery rather than migrating the repo-wide lock type. [skip-regression-check]
Replaces nine INTERNAL_SERVER_ERROR mappings that passed raw SecretsStore / HMAC error strings through to the user; the new internal_err helper logs the detail via tracing::error! and surfaces a generic operation message per the channel-boundary rule. Adds assert_no_delimiter at the verify_signed_install and register entry points to reject any payload field containing ':' so future fields admitting colons cannot create a canonical-serialization ambiguity. Three regression tests cover the helper, the delimiter check, and verify_signed_install through the caller.
|
there is a reborn version of this as a separate PR now. |
|
Yeah, that's #4479, I'm tracking it. I've been building on that branch: ported the deep-link register/install gateway onto the Reborn surface and added the private-manifest install path, with agent-side e2e passing against staging. I'll open it as a sub-PR into |
3 participants
Summary
ironclaw ironhub install/search/list/info) and as agent-callable tools, so a running agent can install at runtime instead of only at build time.tools.jsonplus the live moderated Iliad catalog) with a per-entry provenance tier, defaulting to the least-trusted tier when the field is absent (fail-closed).Change Type
Linked Issue
None. This is net-new work brought to the team directly; happy to file a tracking issue if the team prefers one on record.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings(zero warnings)cargo build/cargo check --no-default-features --features libsqlcargo test --lib(5205 passing, 0 failed), including the signed-manifest envelope verify suite (valid vector, tampered manifest, wrong key, unknown key id, embedded-key validity), the--release-tagrejection test, and the existing IronHub installer, provenance, host-allowlist, dispatch, and gateway handler suitescargo test --features integration— N/A; no database-backed or integration behaviour changed on this sideSecurity Impact
Yes. The catalog manifest is now Ed25519-signed and verified against an embedded public key before use, fail-closed: the agent will not install from an unsigned or tampered manifest, and the signing private key is held only by the IronHub server, never in this repo or binary. New outbound fetches (catalog manifest + artifacts) are constrained to a closed host allowlist (
hub.ironclaw.complus GitHub release infrastructure), HTTPS-only, IP literals and internal/bare hosts rejected. Every artifact is SHA-256-verified before write. Agent-callable install tools use strict parameter schemas (regex +additionalProperties:false), per-tool rate limiting, and an approval gate; unverified community content requires an explicit acknowledgement. The deep-link path is HMAC-verified with a short replay window and a one-shot nonce; signing keys are per-user in the secrets store. No credential is exposed to tool processes.Database Impact
None. No migrations or schema changes on the ironclaw side.
Blast Radius
Registry installer, tool registry/dispatch, and the gateway web handlers + embedded SPA. The feature is additive (new tools/endpoints/surface). The behavioural changes are: the default IronHub manifest URL, the requirement that the manifest be signed (fail-closed), and
--release-tagreturning a clear error instead of fetching an unsigned pinned manifest.Rollback Plan
Revert the branch. The change is additive with no migrations and no persisted state, so reverting removes the new tools, endpoints, default-URL change, and signature requirement cleanly with no data cleanup.
Review Follow-Through
The HMAC deep-link receiver is complete and tested on this side; the matching IronHub-side "Install to Agent" sender and profile key field is a separate follow-up in the IronHub repo and not part of this PR. Iliad-sourced entries are intentionally surfaced as
verified(notofficial);officialis reserved for GitHub-release artifacts.--release-tag(pinning to a specific GitHub release) is disabled in this PR. It fetched an unsigned manifest directly from GitHub, which fail-closed verification rejects, so it now returns a clear error pointing at the default signed catalog. No user-facing capability is lost: the default catalog path (CLI and deep-link) already installs every published tool and skill, signed. Restoring pinning cleanly means routing--release-tagthrough the catalog endpoint with a tag parameter so the server signs on the fly, keeping the key server-only; this is tracked as a follow-up rather than signing in CI, which would require a second copy of the private key in GitHub Actions.Happy to adjust scope or split commits if that helps review.
Review track: C (security/runtime)