release(v0.43.0): SEP-2787 attestation + execution-receipt pairing in proxy#168
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR introduces SEP-2787 attestation and execution-receipt pairing for the Vaara MCP proxy, emitting cryptographically linked JSON artifacts per tool call with signature verification and back-link validation, controlled via new CLI flags, and comprehensively tested end-to-end. ChangesSEP-2787 Attestation and Receipt Pairing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def _build_attest_emitter_from_args( | ||
| args: argparse.Namespace, | ||
| *, | ||
| upstreams: dict[str, list[str]], | ||
| ) -> Optional[AttestPairEmitter]: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/vaara/integrations/_mcp_attest.py`:
- Around line 298-307: When detecting an EllipticCurvePrivateKey in
build_attest_emitter() where you set alg = "ES256", add a check that the
key.curve is the P-256 curve (secp256r1 / ec.SECP256R1) and if not raise
AttestConfigError with a clear message requiring secp256r1; reference
sign_es256() in the message or comment so it's clear ES256 signing expects
32-byte r/s values, and note that AttestPairEmitter.emit_attestation() currently
swallows signing errors so we must reject unsupported curves up front.
In `@src/vaara/integrations/mcp_proxy.py`:
- Around line 1396-1400: The emitter is being built with the raw upstreams dict
keyed by the operator name (e.g. "github") which mismatches the proxy's
collapsing to "default" for a single upstream, causing
fingerprint_for(_REQUEST_UPSTREAM) to miss entries; to fix, normalize the
upstreams before calling _build_attest_emitter_from_args so its keys match
VaaraMCPProxy's slot names (when legacy_single is non-None replace the single
upstream key with "default" or otherwise construct normalized_upstreams =
{'default': list(next(iter(upstreams.values())))} and pass normalized_upstreams
to _build_attest_emitter_from_args instead of the original upstreams), ensuring
attest_emitter, legacy_single, and fingerprint_for use the same upstream name
mapping.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a89150b2-b996-4b4d-afcc-de3f1267679c
📒 Files selected for processing (11)
.gitignoreCHANGELOG.mdREADME.mdclients/ts/package.jsonpyproject.tomlserver-vaara-server.jsonserver.jsonsrc/vaara/__init__.pysrc/vaara/integrations/_mcp_attest.pysrc/vaara/integrations/mcp_proxy.pytests/test_integrations_mcp_proxy_attest.py
| if isinstance(key, EllipticCurvePrivateKey): | ||
| alg: str = "ES256" | ||
| elif isinstance(key, RSAPrivateKey): | ||
| alg = "RS256" | ||
| else: | ||
| raise AttestConfigError( | ||
| "--attest-signing-key must be EC P-256 (ES256) or RSA (RS256). " | ||
| "Generate: openssl ecparam -genkey -name prime256v1 | " | ||
| "openssl pkcs8 -topk8 -nocrypt -out attest_key.pem" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the SEP-2787 ES256 signing path for curve handling.
rg -nP -C4 'ES256|SECP256R1|ECDSA|def emit_attestation|def _sign' --type=py -g '!**/test*'Repository: vaaraio/vaara
Length of output: 28818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect ES256 sign/verify implementation details (curve assumptions like fixed 32-byte r||s).
rg -n "def sign_es256|def verify_es256" -S src/vaara/attestation/_sep2787_signing.py
sed -n '1,120p' src/vaara/attestation/_sep2787_signing.py
sed -n '120,220p' src/vaara/attestation/_sep2787_signing.py
# Inspect SEP-2787 emit/verify path for exception swallowing / behavior on sign failures.
rg -n "def emit_attestation|try:|except" -S src/vaara/attestation/_sep2787_emit.py
sed -n '1,240p' src/vaara/attestation/_sep2787_emit.py
# Inspect the MCP integration's emit_attestation to see if it swallows exceptions and produces artifacts.
rg -n "def emit_attestation|try:|except" -S src/vaara/integrations/_mcp_attest.py
sed -n '120,220p' src/vaara/integrations/_mcp_attest.py
sed -n '220,380p' src/vaara/integrations/_mcp_attest.pyRepository: vaaraio/vaara
Length of output: 17566
Enforce P-256 (secp256r1) for ES256 auto-detection
build_attest_emitter() labels any EllipticCurvePrivateKey as ES256, but sign_es256() always encodes r and s as fixed 32-byte values for the SEP-2787 ES256 wire format. For non-P-256 curves this can raise during signing, and AttestPairEmitter.emit_attestation() then catches the exception, logs it, returns None, and does not persist an attestation (or receipt). Reject non-secp256r1 keys up front with a clear AttestConfigError.
🛡️ Proposed curve check
if isinstance(key, EllipticCurvePrivateKey):
- alg: str = "ES256"
+ from cryptography.hazmat.primitives.asymmetric import ec
+ if not isinstance(key.curve, ec.SECP256R1):
+ raise AttestConfigError(
+ "--attest-signing-key EC key must use the P-256 (secp256r1) "
+ f"curve for ES256; got {key.curve.name!r}."
+ )
+ alg: str = "ES256"
elif isinstance(key, RSAPrivateKey):
alg = "RS256"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(key, EllipticCurvePrivateKey): | |
| alg: str = "ES256" | |
| elif isinstance(key, RSAPrivateKey): | |
| alg = "RS256" | |
| else: | |
| raise AttestConfigError( | |
| "--attest-signing-key must be EC P-256 (ES256) or RSA (RS256). " | |
| "Generate: openssl ecparam -genkey -name prime256v1 | " | |
| "openssl pkcs8 -topk8 -nocrypt -out attest_key.pem" | |
| ) | |
| if isinstance(key, EllipticCurvePrivateKey): | |
| from cryptography.hazmat.primitives.asymmetric import ec | |
| if not isinstance(key.curve, ec.SECP256R1): | |
| raise AttestConfigError( | |
| "--attest-signing-key EC key must use the P-256 (secp256r1) " | |
| f"curve for ES256; got {key.curve.name!r}." | |
| ) | |
| alg: str = "ES256" | |
| elif isinstance(key, RSAPrivateKey): | |
| alg = "RS256" | |
| else: | |
| raise AttestConfigError( | |
| "--attest-signing-key must be EC P-256 (ES256) or RSA (RS256). " | |
| "Generate: openssl ecparam -genkey -name prime256v1 | " | |
| "openssl pkcs8 -topk8 -nocrypt -out attest_key.pem" | |
| ) |
🤖 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 `@src/vaara/integrations/_mcp_attest.py` around lines 298 - 307, When detecting
an EllipticCurvePrivateKey in build_attest_emitter() where you set alg =
"ES256", add a check that the key.curve is the P-256 curve (secp256r1 /
ec.SECP256R1) and if not raise AttestConfigError with a clear message requiring
secp256r1; reference sign_es256() in the message or comment so it's clear ES256
signing expects 32-byte r/s values, and note that
AttestPairEmitter.emit_attestation() currently swallows signing errors so we
must reject unsupported curves up front.
| attest_emitter = _build_attest_emitter_from_args(args, upstreams=upstreams) | ||
|
|
||
| legacy_single = ( | ||
| list(next(iter(upstreams.values()))) if len(upstreams) == 1 else None | ||
| ) |
There was a problem hiding this comment.
Emitter fingerprint table is keyed under the wrong upstream name for a named single upstream.
The emitter is built with the raw upstreams dict (keyed by the operator-supplied name, e.g. github), but when there is exactly one upstream, legacy_single is non-None and VaaraMCPProxy.__init__ collapses it into the "default" slot. At runtime _REQUEST_UPSTREAM resolves to "default" for the single-upstream case (a named X-Vaara-Upstream header even 404s, since only "default" exists), so fingerprint_for("default") misses the precomputed cmd:sha256:... entry stored under github and falls back to the cmd:sha256:unknown-default placeholder. Any tools/call before the first tools/list upgrade therefore carries a bogus server fingerprint. A bare --upstream CMD (keyed default) and multi-upstream fan-out are unaffected; only the documented --upstream NAME=CMD single-upstream form is.
Normalize the names the emitter is built with to match how the proxy slots them.
🐛 Proposed fix
- attest_emitter = _build_attest_emitter_from_args(args, upstreams=upstreams)
-
- legacy_single = (
- list(next(iter(upstreams.values()))) if len(upstreams) == 1 else None
- )
+ legacy_single = (
+ list(next(iter(upstreams.values()))) if len(upstreams) == 1 else None
+ )
+ # The proxy collapses a single upstream into the "default" slot, so the
+ # attestation emitter must key its fingerprint table the same way or
+ # fingerprint_for("default") misses the precomputed cmd-hash.
+ attest_upstreams = (
+ {"default": legacy_single} if legacy_single is not None else upstreams
+ )
+ attest_emitter = _build_attest_emitter_from_args(args, upstreams=attest_upstreams)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| attest_emitter = _build_attest_emitter_from_args(args, upstreams=upstreams) | |
| legacy_single = ( | |
| list(next(iter(upstreams.values()))) if len(upstreams) == 1 else None | |
| ) | |
| legacy_single = ( | |
| list(next(iter(upstreams.values()))) if len(upstreams) == 1 else None | |
| ) | |
| # The proxy collapses a single upstream into the "default" slot, so the | |
| # attestation emitter must key its fingerprint table the same way or | |
| # fingerprint_for("default") misses the precomputed cmd-hash. | |
| attest_upstreams = ( | |
| {"default": legacy_single} if legacy_single is not None else upstreams | |
| ) | |
| attest_emitter = _build_attest_emitter_from_args(args, upstreams=attest_upstreams) |
🤖 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 `@src/vaara/integrations/mcp_proxy.py` around lines 1396 - 1400, The emitter is
being built with the raw upstreams dict keyed by the operator name (e.g.
"github") which mismatches the proxy's collapsing to "default" for a single
upstream, causing fingerprint_for(_REQUEST_UPSTREAM) to miss entries; to fix,
normalize the upstreams before calling _build_attest_emitter_from_args so its
keys match VaaraMCPProxy's slot names (when legacy_single is non-None replace
the single upstream key with "default" or otherwise construct
normalized_upstreams = {'default': list(next(iter(upstreams.values())))} and
pass normalized_upstreams to _build_attest_emitter_from_args instead of the
original upstreams), ensuring attest_emitter, legacy_single, and fingerprint_for
use the same upstream name mapping.
2 participants
Summary
_mcp_attest.py:AttestPairEmitteremits a SEP-2787 request attestation and an execution receipt per allowedtools/call; paired JSON files go to a configurable receipts directory--attest-signing-key+--attest-receipts-dirflags onvaara-mcp-proxy, off by default; key type is auto-detected: EC P-256 PEM = ES256, RSA PEM = RS256, raw bytes = HS256serverFingerprintstarts as a SHA-256 of the upstream command string and upgrades to a manifest hash on the firsttools/listresponse, capturing which tools the proxy actually exposedX-Vaara-IntentHTTP header lets operators supply a richer intent label per call; stdio falls back totools/call/{tool_name}backLinkdigest over the full attestation wire bytes; a verifier can confirm the pair belongs together without trusting the proxyerroredreceipt is still written so no attestation is left orphaneda5f19aa)server.json+server-vaara-server.jsonto 0.43.0 alongside pyproject,__init__.py, and the npm package (five-surface version parity)Test plan
pip install vaara==0.43.0resolves on PyPI after Release workflownpm install @vaara/client@0.43.0resolves--attest-signing-key+--attest-receipts-dir, call a tool, confirm paired*-attest.json/*-receipt.jsonwritten andverify_back_linkpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--attest-signing-keyand--attest-receipts-dirCLI flags to enable attestation (off by default).X-Vaara-IntentHTTP header support for intent override.Chores
.gitignoreto exclude local development artifacts.