Feat/tool broker extraction phase1 - #1859
Conversation
…tool-broker binary Phase 1 of the tool-broker extraction: the gateway process no longer links aegis-tool-broker-connectors (removed from src/Cargo.toml production deps, kept as a dev-dependency for the one deliberate mock-server test exception). It never resolves a real provider credential or runs a shell command itself anymore. POST /v1/broker/execute now calls the new standalone aegis-tool-broker binary over HTTP with a required service-to-service bearer token (AEGIS_TOOL_BROKER_API_TOKEN); with no broker configured it fails closed with 501, matching the OIDC/signed-policy-bundle precedent. The gateway is unchanged for authorization, hash-bound approval consumption, and receipt emission -- only the connector-execution step moved process. Ships with a Helm chart (helm/aegis-tool-broker/) and a docker-compose.full.yml service, both wired for local/dev use. Known Phase-1 gap (documented in Tool_Broker.md's Honest Scope, not glossed over): the gateway consumes the approval before the broker HTTP call, so a broker-unreachable failure burns the approval without executing the action. Covered by execute_fails_closed_when_the_broker_is_unreachable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Updated README.md to include links to new architecture and migration documents. - Modified architecture-map.json to reflect changes in the SOC Console Dashboards description and file paths. - Revised architecture.md to establish mandatory architecture laws and clarify dependency flows and integrity laws. - Updated mkdocs.yml to include new architecture law and low-level design documents in the navigation. - Improved audit-doc-quality.mjs to ensure no trailing newlines in rendered output. - Added LLD.md to the list of required documents in validate-docs.mjs for documentation validation.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)src/src/main.rsast-grep timed out on this file 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 |
![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 extracts the tool broker from the gateway into a standalone aegis-tool-broker binary (Phase 1 extraction), removing the connector and credential execution engine from the gateway process to enhance security. It also introduces the target high-level and low-level designs, a migration matrix, and a 36-week roadmap. The code review identified several critical improvements: a timing attack vulnerability in the token comparison function that requires a constant-time comparison, an unbounded response body read in the gateway's client that should be fixed by setting a timeout on the HTTP client builder, a non-portable healthcheck host (0.0.0.0) that should be replaced with 127.0.0.1, and a misleading optional: true flag in the Helm secret reference for the required API token.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
bins/aegis-tool-broker/src/auth.rs (18-24)
The tokens_match function compares hex-encoded SHA-256 digests using a standard non-constant-time string comparison (==). This is vulnerable to a timing attack that can leak the characters of the hash, allowing an attacker to reconstruct the hash and perform offline brute-force attacks. To prevent this, compare the raw binary digests using a constant-time comparison algorithm.
pub fn tokens_match(configured: &str, provided: &str) -> bool {
use sha2::{Digest, Sha256};
let conf_hash = Sha256::digest(configured.as_bytes());
let prov_hash = Sha256::digest(provided.as_bytes());
// Constant-time comparison of the 32-byte digests to prevent timing attacks
let mut accum = 0;
for (a, b) in conf_hash.iter().zip(prov_hash.iter()) {
accum |= a ^ b;
}
accum == 0
}src/src/tool_broker_client.rs (67-74)
The reqwest::Client is created without a timeout, and tokio::time::timeout only wraps the .send() call. This means the subsequent reading of the response body (via .json().await) is completely unbounded. If the tool broker hangs or transmits the body extremely slowly, the gateway thread/task will block indefinitely, potentially leading to resource exhaustion. Configuring the timeout on the reqwest::Client itself ensures the entire request lifecycle, including the body read, is bounded.
pub fn new(base_url: String, api_token: String, timeout_secs: u64) -> Self {
let timeout = Duration::from_secs(timeout_secs);
let http = reqwest::Client::builder()
.timeout(timeout)
.build()
.unwrap_or_else(|_| reqwest::Client::new());
Self {
execute_url: format!("{base_url}/v1/execute"),
api_token,
http,
timeout,
}
}bins/aegis-tool-broker/src/main.rs (50-56)
The healthcheck command attempts to connect directly to cli.listen, which defaults to 0.0.0.0:8899. Connecting to 0.0.0.0 is non-portable and fails on macOS and certain Linux network configurations. It is safer and more robust to replace 0.0.0.0 with 127.0.0.1 for local healthcheck requests.
if cli.healthcheck {
let host = if cli.listen.starts_with("0.0.0.0:") {
cli.listen.replace("0.0.0.0:", "127.0.0.1:")
} else {
cli.listen.clone()
};
let url = format!("http://{host}/livez");
return match reqwest::get(&url).await {
Ok(resp) if resp.status().is_success() => Ok(()),
_ => std::process::exit(1),
};
}
helm/aegis-tool-broker/templates/deployment.yaml (47-52)
The AEGIS_TOOL_BROKER_API_TOKEN environment variable is strictly required for the tool broker binary to start. Setting optional: true on the secretKeyRef is misleading because if the secret or key is missing, the container will still start but immediately crash-loop. Removing optional: true (or setting it to false) allows Kubernetes to block the deployment and surface a clear "missing secret" event, which is much easier to diagnose.
- name: AEGIS_TOOL_BROKER_API_TOKEN
valueFrom:
secretKeyRef:
name: {{ include "aegis-tool-broker.secretName" . }}
key: AEGIS_TOOL_BROKER_API_TOKEN
Summary
Closes #
Type of change
Checklist
cargo test --workspaceandpython3 -m unittest discover -s sdk-python/testspass.cargo fmt --all -- --checkandcargo clippy --workspace --all-targets -- -D warningspass.python3 -m black --check sdk-python/ examples/passes.tenant_id; parameterized SQL only.feat:,fix:,docs:, etc.).Integrity invariants (do not weaken)
Notes for reviewers